Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UI] Dynamically adding front-matter to documentation pages #379

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

SAHU-01
Copy link
Contributor

@SAHU-01 SAHU-01 commented Sep 30, 2024

Notes for Reviewers

This PR fixes #89

  • It adds front-matter to respective documented pages sharing the subscription tier this feature is available to.
  • This is done dynamically with populating data from the pricing-list workflow and based on the data received, a go script adds the front-matter to their respective documented pages.
image

Signed commits

  • Yes, I signed my commits.

Copy link

netlify bot commented Sep 30, 2024

Deploy Preview for bejewelled-pegasus-b0ce81 ready!

Name Link
🔨 Latest commit 175d72f
🔍 Latest deploy log https://app.netlify.com/sites/bejewelled-pegasus-b0ce81/deploys/67163b592529fa0008f0c44f
😎 Deploy Preview https://deploy-preview-379--bejewelled-pegasus-b0ce81.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@vishalvivekm
Copy link
Member

vishalvivekm commented Sep 30, 2024

// @lekaf974


- name: Wait for Pricing List workflow to complete
run: |
# You may need to implement a polling mechanism here to check the status of the other workflow

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we consider that the other workflow trigger this one once done ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can have the other workflow trigger this one when it's done, rather than relying on a polling mechanism.

- name: Pull latest changes
run: |
git pull origin master

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What the reason to pull master since a previous step is already doing a checkout ?

Copy link
Contributor Author

@SAHU-01 SAHU-01 Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for pulling from master after the checkout step is because the first workflow (which generates the pricing-data.json) pushes changes to the repository, so when the current workflow runs, it needs to pull the updated data to ensure it has the latest pricing-data.json. This ensures the front matter is updated based on the most recent pricing information generated by the other workflow.

But you're right! Since we are performing the checkout after triggering the other workflow, the newly added or modified .json file from the "generate-pricing-list.yml" workflow should already be included when the checkout step is executed.


func main() {
// Read the JSON file
jsonFile, err := ioutil.ReadFile("pricing_data.json")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From where the file is coming ?

Could we also use a environment variable instead of hardcoded value for the file name ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is added from the "generate-pricing-list.yml" workflow call.

Yes, we can absolutely use an environment variable, I'll add one, Thanks!


// Extract hash if present
urlHash := ""
for i, part := range urlParts {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather put this logic in a separate method with a extractHah to make code more readable and to remove the comment

}

// Build the path step by step
for _, part := range urlParts {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same buildPath


if urlHash == "" {
// No hash: add shortcode after the front matter
frontMatterEnd := strings.Index(contentStr, "---")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here having a method with proper name for readability

}
} else {
// Hash present: search for matching heading and add shortcode below
hashPattern := strings.ReplaceAll(urlHash, "-", "[-\\s]")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here having a method with proper name for readability

Co-authored-by: Matthieu Evrin <[email protected]>
Signed-off-by: Lee Calcote <[email protected]>
@leecalcote
Copy link
Member

Suggested changes accepted.

return
}

// Group entries by documentation URL
Copy link

@lekaf974 lekaf974 Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SAHU-01

You can remove all the comments since just reading the code is enough to understand what it is doing.

Usually comments are used for developers documentation or if the algorithm is doing something complex or we need to have context to understand which is not the case here.

Otherwise LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll get rid of all the comments, Thanks!

@lekaf974
Copy link

lekaf974 commented Oct 7, 2024

@sudhanshutech

I did reviews on go script but could you review the html css part I'm less comfortable on this

@leecalcote
Copy link
Member

It's coming along!

@vishalvivekm
Copy link
Member

vishalvivekm commented Oct 14, 2024

@SAHU-01
will you push these changes ( with the not working svgs):
https://www.youtube.com/watch?v=L2bG-kfR5u4&t=2868s

@vishalvivekm
Copy link
Member

@SAHU-01
Let's discuss the recent changes and update on the item today, during the website call at 5:30 PM IST (7:00 AM CT).

Please add it as an agenda item to the meeting minutes.

Signed-off-by: Ankita Sahu <[email protected]>
@vishalvivekm
Copy link
Member

@SAHU-01 Let's discuss the recent changes and update on the item today, during the website call at 5:30 PM IST (7:00 AM CT).

Please add it as an agenda item to the meeting minutes.

adding

Signed-off-by: Ankita Sahu <[email protected]>
@SAHU-01
Copy link
Contributor Author

SAHU-01 commented Oct 21, 2024

This flowchart explains how the workflows are working: https://playground.meshery.io/extension/meshmap?mode=design&design=cbb68338-be01-4e0c-b833-3b88c024ca61

script: |
const result = await github.rest.actions.createWorkflowDispatch({
owner: 'layer5labs',
repo: 'meshery-extensions-packages',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of running the workflow over here? @vishalvivekm @SAHU-01 @jerensl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Create a "Who Can Use This Feature" layout/block/shortcode
4 participants