[GitHub] [superset] kgabryje commented on a diff in pull request #22682: fix(nav): make doc and bug buttons customizable

[GitBox <gi...@apache.org>]

kgabryje commented on code in PR #22682:
URL: https://github.com/apache/superset/pull/22682#discussion_r1067106016


##########
superset-frontend/src/views/components/RightMenu.tsx:
##########
@@ -539,25 +539,38 @@ const RightMenu = ({
         )}
       </Menu>
       {navbarRight.documentation_url && (
-        <StyledAnchor
-          href={navbarRight.documentation_url}
-          target="_blank"
-          rel="noreferrer"
-          title={t('Documentation')}
-        >
-          <i className="fa fa-question" />
-          &nbsp;
-        </StyledAnchor>
+        <>
+          <StyledAnchor
+            href={navbarRight.documentation_url}
+            target="_blank"
+            rel="noreferrer"
+            title={navbarRight.documentation_text || t('Documentation')}

Review Comment:
   1. Should we translate `documentation_text`? If not, we probably don't want to translate `bug_report_text` in line 564?
   2. Are you sure that `t()` will work correctly with dynamic expression like here (text depends on result of `||` operator). Shouldn't we use `t('%s', navbarRight.documentation_text || 'Documentation')` instead? That's actually more of a question than suggestion, I'm not sure which style is preferred here



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org