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

[#12271] Docs: Upgrade to latest MarkBind version #12893

Merged
merged 11 commits into from
Mar 16, 2024

Conversation

jingting1412
Copy link
Contributor

@jingting1412 jingting1412 commented Mar 12, 2024

Fixes #12271

Outline of Solution
Upgraded the docs to the latest version of Markbind, v5.3.0 (full release notes can be found here).

I have deployed it from my branch at this link

UI changes:

  • Added breadcrumbs to the pages for users to see the hierarchy of the site
Screenshot 2024-03-13 at 1 50 47 PM

Other changes:

  • Made navbar sticky so that it remains at the top of the page while scrolling. From testing on my deployed branch, using <header fixed> would sometimes not cause the header to remain at the top while scrolling, shown in the screenshot below which is at this location on the page:
Screenshot 2024-03-13 at 4 58 54 PM

As the screenshot shows, the header is not present when I've scrolled down the page. As such I've changed it to <header sticky> to ensure that it would always stick to the top while scrolling

  • Changed how style.puml is included in the puml diagrams. From testing on my own deployed branch, the include statement !include diagrams/style.puml would sometimes cause the path to be resolved wrongly as shown in this commit. When I changed the statement to just !include style.puml, the diagrams are correctly shown as seen in this commit

@kaixin-hc
Copy link
Contributor

Thanks for working on this @jingting1412 ! Would you revise the issue description so it is easier for teammates people to review? I think screenshots/video will help :)

Comparing the new appearance against the old, I think that the sticky header and working PUML are actually already in the live dev docs - so the only "change" in the UI is the breadcrumbs (as auto-breadcrumbs being a MarkBind feature added in 5.0). Is that correct? (if it is, maybe can separate out the UI change from your explanations of why the other code changed 😅

I understand that the variables file is not used on the teammates website and variables.json does not exist. Perhaps we should remove this file? (TEAMMATES people, if this is considered an unrelated change we can also file an issue and open a separate PR - let us know!)

@jingting1412
Copy link
Contributor Author

thanks @kaixin-hc for the suggestion! I've done more testing on my deployed branch and have since updated the description to better reflect the nature of the changes and to add images.

Regarding the variables.md file, I think its up to the TEAMMATES team if they still want to keep it or not. Its a useful tool to have in case they would like to declare variables in the future. You can read more about it in our user guide here

Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for providing a deployed version of the docs for us to look at!

@cedricongjh cedricongjh added the s.FinalReview The PR is ready for final review label Mar 14, 2024
Copy link
Contributor

@ziqing26 ziqing26 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the change! It is good to keep the variable file in case we need to declare some in the future.

@ziqing26 ziqing26 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Mar 14, 2024
package.json Outdated
@@ -75,6 +75,7 @@
"eslint-plugin-rxjs": "^5.0.3",
"jest": "^29.7.0",
"lintspaces-cli": "^0.7.1",
"markbind-cli": "^5.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed. Only the package.json file in docs/ folder needs the change.

Copy link
Member

Choose a reason for hiding this comment

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

@jingting1412 I believe you have figured it out, but the main project (thus root package.json) has nothing to do with MarkBind. This is not a Lerna monorepo project whereby the root package.json affects the one in the docs folder.

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 understood thank you, I have since reverted the changes I've made to the main json files.

@wkurniawan07 wkurniawan07 added s.Ongoing The PR is being worked on by the author(s) and removed s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging labels Mar 15, 2024
@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.Ongoing The PR is being worked on by the author(s) labels Mar 15, 2024
@cedricongjh cedricongjh merged commit 2d10806 into TEAMMATES:master Mar 16, 2024
11 checks passed
@cedricongjh cedricongjh added this to the V9.0.0-beta.1 milestone Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: Upgrade to latest MarkBind version
5 participants