-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
fix(docs): ensure version dropdown sorts correctly regardless of browser language #20289 #20322
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
7af38ac
to
c269a07
Compare
c269a07
to
3cb9292
Compare
docs/assets/versions.js
Outdated
const link = dd.querySelector('a'); | ||
if (link) { | ||
// Check if the link's href matches version pattern | ||
return /\/en\/(release-(?:v?\d+[\.\d+]*)|\d+\.\d+\.\d+|latest|stable)\//.test(link.getAttribute('href')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argo-cd/docs/assets/versions.js
Line 23 in 3cb9292
const currentVersion = window.location.href.match(/\/en\/(release-(?:v\d+|[\d\.]+|\w+)|latest|stable)\//); |
The version regex expression Line 23 excludes versions without the release- prefix, except for 'latest' or 'stable'. However,Here currently including them. Why is this necessary? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now corrected it. I appreciate your help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. I'd like to present another small request related to this -
Right now, if I select any version on the docs, the topbar doesn't reflect which version I'm currently at completely. As you can see in the below image, it shows as Release..... You can either put those changes in the same PR or raise a new PR for it.
3cb9292
to
9da4cbc
Compare
Hello, @nitishfy. I have reviewed what you mentioned. I will address the issue and submit a new PR shortly. Thank you! |
docs/assets/versions.js
Outdated
if (link) { | ||
// Check if the link's href matches version pattern | ||
return /\/en\/(release-(?:v\d+|[\d\.]+|\w+)|latest|stable)\//.test(link.getAttribute('href')); | ||
} | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (link) { | |
// Check if the link's href matches version pattern | |
return /\/en\/(release-(?:v\d+|[\d\.]+|\w+)|latest|stable)\//.test(link.getAttribute('href')); | |
} | |
return false; | |
return /\/en\/(release-(?:v\d+|[\d\.]+|\w+)|latest|stable)\//.test(link?.getAttribute?.('href')); |
This could be simplified.
If the version regex expression is the same as above, maybe we should store it in a constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used a constant for the regex as you suggested. Thanks!
…ser language Signed-off-by: jaehanbyun <awbrg789@naver.com>
9da4cbc
to
0281864
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ser language argoproj#20289 (argoproj#20322) Signed-off-by: jaehanbyun <awbrg789@naver.com> Signed-off-by: Adrian Aneci <aneci@adobe.com>
Closes #20289
Checklist:
What I have done:
I resolved an issue where 'version' was hardcoded, preventing proper sorting in browsers set to languages other than English.
The screenshot below shows that it now operates correctly in a browser set to German.