- 
                Notifications
    
You must be signed in to change notification settings  - Fork 38
 
Basebutton tooltip params #1402
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
base: develop
Are you sure you want to change the base?
Conversation
| 
           Use  Run test server using develop.opencast.org as backend: Specify a different backend like stable.opencast.org: It may take a few seconds for the interface to spin up.  | 
    
5ce319c    to
    3aece8c      
    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.
Can confirm that this fixes the issue, thanks for that! Since the bug is already occurring in r/17.x, could you rebase your branch onto r/17.x and point the PR against it instead of develop? (Since very recently we now have the same versioning in the admin interface as we have in Opencast. r/17.x is the oldest and we forward merge our versions, so if it goes into r/17.x it will go into all versions.)
| <Tooltip title={tooltipParams ? t(tooltipText) : t(tooltipText, tooltipParams)}> | ||
| {buttonComponent} | ||
| <Tooltip title={tooltipParams ? t(tooltipText, tooltipParams) : t(tooltipText)}> | ||
| {buttonComponent} | 
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.
This looks like an accidental removal to me?
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.
This wasn’t accidental. I updated the expression to tooltipParams ? t(tooltipText, tooltipParams) : t(tooltipText) because the previous version caused an error. The change ensures that tooltipParams are passed to t() only when they exist, otherwise it falls back to the plain translation string.
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 see know that the line I marked did not all make clear what I wanted to refer to. I am very sorry for the confusion, of course updating the expression as you did makes perfect sense. I was only referring to the removal of the tab character ^^`.
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.
ok got it
This PR fixes an issue where tooltipParams were ignored when rendering the tooltip in BaseButton.
Previously, placeholders like {{filterName}} in translations were not being replaced even when tooltipParams was provided.