-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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(nav): make doc and bug buttons customizable #22682
Conversation
Codecov Report
@@ Coverage Diff @@
## master #22682 +/- ##
==========================================
- Coverage 67.14% 67.10% -0.04%
==========================================
Files 1869 1869
Lines 71523 71586 +63
Branches 7814 7814
==========================================
+ Hits 48022 48040 +18
- Misses 21460 21505 +45
Partials 2041 2041
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
href={navbarRight.documentation_url} | ||
target="_blank" | ||
rel="noreferrer" | ||
title={navbarRight.documentation_text || t('Documentation')} |
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.
- Should we translate
documentation_text
? If not, we probably don't want to translatebug_report_text
in line 564? - Are you sure that
t()
will work correctly with dynamic expression like here (text depends on the result of||
operator). Shouldn't we uset('%s', navbarRight.documentation_text || 'Documentation')
instead? That's actually more of a question than suggestion, I'm not sure which style is preferred here
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.
Oof, the bug_report_text
translation was a complete mistake from me! Will fix
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.
Regarding question 2, I think the pattern in documentation_text
is correct, i.e. we want to use the string from the config as-is, and only fall back to the translated text if it's undefined.
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!
SUMMARY
Currently the documentation text and icon config parameters are not being respected in the
RightMenu
component. See the parameters here:superset/superset/config.py
Lines 1307 to 1310 in c0aeb2a
This fixes that + adds similar overrides for the bug links. In addition, the old right menu FAB template is no longer being used, and can hence be deleted.
AFTER
Here's the docs and buglinks with the new customizations (notice the top right hand corner next to "Settings"):
BEFORE
Previously the
DOCUMENTATION_ICON
andDOCUMENTATION_ICON
params weren't being used, and the bug link was not customizable:TESTING INSTRUCTIONS
ADDITIONAL INFORMATION