Fix Navigation Locale#3076
Conversation
|
Please find a preview at: https://staging.nodejs.dev/3076/ |
|
@RavenColEvol please run |
|
@benhalverson I've ran the format command and merged the main changes please review and let me know if any changes required. |
|
Please find a preview at: https://staging.nodejs.dev/3076/ |
Codecov ReportBase: 66.02% // Head: 67.59% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #3076 +/- ##
==========================================
+ Coverage 66.02% 67.59% +1.57%
==========================================
Files 118 146 +28
Lines 1351 1682 +331
Branches 342 392 +50
==========================================
+ Hits 892 1137 +245
- Misses 422 500 +78
- Partials 37 45 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
These changes are super cool, but sadly are problematic as you're doing the language filtering in the client-side. The actual "data" of the navigation is after build-time stored in a JSON file for each page called "page-data.json", I can see this file increasing its size tenfold for every language that we support 😅 Could you maybe think about a way where the filtering of the language could be done within the nodeCreation? |
|
@ovflowd Can you please check now |
|
@benhalverson can you add the preview tag? |
|
Have you added a way to translate the category titles? |
|
@AugustinMauroy right now we don't store the title translation in learnEdges that's why I've only added it for subparts. |
|
Nice! |
|
Please find a preview at: https://staging.nodejs.dev/3076/ |
|
@RavenColEvol I'm going to give a deeper review as soon as I'm able to, sorry 😅 |
AugustinMauroy
left a comment
There was a problem hiding this comment.
Great work! I don't see any mistakes.
Thanks for your first contribution! Welcome
Sure @ovflowd |
|
@ovflowd did you get a chance to review the code? |
|
Does it look like I had a chance? 😅 (Have patience please. There’s no rush for releasing this 🙃) |
ovflowd
left a comment
There was a problem hiding this comment.
Overall a nice change, I believe there are things that can be improved if you revise your logic complexity.
|
@ovflowd tried to refactor the code by creating more utility functions. |
|
Thanks! Will give an 👀 |
ovflowd
left a comment
There was a problem hiding this comment.
Honestly, the code looks good enough for an initial version! Thank you so much for all your hard work! 💖
Let's get this merged!
Description
This PR targets the issue of Localisation of Navigation bar and support for localised navigation text for navigation bar.
Related Issues
Fixed #2964 #3046
Check List
npm run lint:js -- --fixand/ornpm run lint:md -- --fixfor my JavaScript and/or Markdown changes.npm run testto check if all tests are passing, and/ornpm run test -- -uto update snapshots if I created and/or updated React Components.npm run buildwork fine.