DOP-4715: Upgrade consistent nav to "v2.1.0" #1108
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rayangler
approved these changes
Jun 3, 2024
There was a problem hiding this comment.
LGTM with one non-blocking nit!
Might need to update tests? Not so sure, would love input on that
I think we can create a test for onSelectLocale if we want to be safe, but I also think behavior is straightforward enough. localizePath does most of the heavy lifting and it's already tested
src/utils/locale.js
Outdated
Comment on lines
145
to
146
| const location = window.location; | ||
| if (isBrowser) { |
There was a problem hiding this comment.
(Nit) window is a browser API and thinking about it now, I think this function would only be called on a browser, so I'm actually wondering if we need this check for isBrowser? I think it would make sense to either remove the unnecessary condition, or move const location = window.location to be within the condition if we're sure we need the isBrowser check for consistency
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stories/Links:
DOP-4715
Current Behavior:
Cloud docs landing page
Staging Links:
English cloud-docs landing page (has Japanese language available in dropdown)
"Japanese" cloud-docs landing page (only navigable from English page)
"Portuguese" cloud-docs landing page
"Korean" cloud-docs landing page
"Simplified Chinese" cloud-docs landing page
You can switch between these pages using the header or footer (will need to append
index.htmlafter the switch). The footer/header should match and correspond to the current link.Notes:
The original request of this ticket was to upgrade the @mdb/consistent-nav package to v2.1.0 so the Language Selector would be available in the header. However, it was quite difficult to get the selector to only display the four languages supported by docs (English, Portuguese, Korean, Simplified Chinese)—fortunately, the web team is including a feature to enable that in v2.1.1-rc.1 (see ticket comment).
Therefore, this PR is updating the consistent nav to v2.1.1-rc.1 to make use of the aforementioned feature, as well as adds support for locale changes to the header. The
onSelectLocalefunction was also moved to thelocale.jsfile to reduce duplication.You can test locally by setting
COMMIT_HASHas one of theenabledLocales(en-us,pt-br,ko-kr,zh-cn) in your.env.*files.Might need to update tests? Not so sure, would love input on that
README updates