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

feat(macro/MDNSidebar): rewrite macro + add missing pages #10329

Merged
merged 8 commits into from
Jan 29, 2024

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar commented Jan 17, 2024

Add all the pages under https://github.com/mdn/content/tree/main/files/en-us/mdn to the macro.

All the documents under the dir hierarchy have been added. Sequence is as per mentions of the pages in coresponding overview pages.

mdnSidebar.mp4

Screenshots

Before change

before

After changes

after

@OnkarRuikar OnkarRuikar requested a review from a team as a code owner January 17, 2024 09:18
@github-actions github-actions bot added the macros tracking issues related to kumascript macros label Jan 17, 2024
@wbamberg
Copy link
Collaborator

wbamberg commented Jan 19, 2024

This looks great! Did you consider using the design in https://github.com/mdn/yari/blob/main/kumascript/macros/LearnSidebar.ejs, which derives link text from page titles wherever possible, to avoid these huge string maps for l10n?

See also #8132.

@OnkarRuikar
Copy link
Contributor Author

@wbamberg I've updated the macro to use titles. It does render well in local environment.

But don't know why the mdnsidebar.test.ts fails. May be because content repo is not checked out and .env is not set. So the code is not able find the page to get the titles. The learnSidebar.ts doesn't have test. Should I delete mdnsidebar.test.ts file?

@caugner
Copy link
Contributor

caugner commented Jan 22, 2024

Should I delete mdnsidebar.test.ts file?

FWIW I think it's okay to remove that test.

@OnkarRuikar
Copy link
Contributor Author

Should I delete mdnsidebar.test.ts file?

FWIW I think it's okay to remove that test.

Done in bbafd23

Also removed from the content kitchensink page: mdn/content#31846

@wbamberg
Copy link
Collaborator

@caugner, please let me know if you're OK for me to review+merge this PR, or if you want to look.

@caugner caugner requested review from a team and pepelsbey and removed request for a team January 23, 2024 19:40
@caugner
Copy link
Contributor

caugner commented Jan 23, 2024

@caugner, please let me know if you're OK for me to review+merge this PR, or if you want to look.

Let me run this by the content team, to be sure.

@OnkarRuikar
Copy link
Contributor Author

I think we need to wait till mdn/content#31868 is fixed. We'll have to update the macro again after those changes are done.

@caugner
Copy link
Contributor

caugner commented Jan 25, 2024

I think we need to wait till mdn/content#31868 is fixed.

mdn/content#31878 fixed that meanwhile.

@OnkarRuikar
Copy link
Contributor Author

@caugner I've updated this PR accordingly. We can merge this now.

@caugner
Copy link
Contributor

caugner commented Jan 26, 2024

@caugner I've updated this PR accordingly. We can merge this now.

@OnkarRuikar Thank you. Can you please add Before/After screenshots to the PR description? This will make it easier to have the change reviewed by the content team.

@OnkarRuikar
Copy link
Contributor Author

@OnkarRuikar Thank you. Can you please add Before/After screenshots to the PR description? This will make it easier to have the change reviewed by the content team.

Done!

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

Had a quick chat with the content team, and all looks good, but can we change the order as follows: 🙏

  • Community guidelines
  • Writing guidelines
  • PAB
  • History

@caugner caugner removed the request for review from pepelsbey January 26, 2024 13:46
@OnkarRuikar
Copy link
Contributor Author

Had a quick chat with the content team, and all looks good, but can we change the order as follows: 🙏

  • Community guidelines
  • Writing guidelines
  • PAB
  • History

This is how the new order looks:
the new orer

@caugner caugner changed the title fix(macro): update MDNSidebar.ejs feat(macro/MDNSidebar): rewrite macro + add missing pages Jan 29, 2024
@caugner caugner merged commit 35d448e into mdn:main Jan 29, 2024
10 checks passed
@OnkarRuikar OnkarRuikar deleted the update_mdn_sidebar branch January 29, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macros tracking issues related to kumascript macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants