-
Notifications
You must be signed in to change notification settings - Fork 276
fix(ui5-button): adjust tooltips display #11268
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
Conversation
🎉 This PR is included in version v2.10.0-rc.2 🎉 The release is available on v2.10.0-rc.2 Your semantic-release bot 📦🚀 |
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.
Pull Request Overview
This PR adjusts the tooltip display behavior on ui5-button so that the default icon tooltip is only applied when the button is icon-only, addressing the UX issue described in #10824.
- Removed the showTooltip property from the ButtonTemplate to simplify tooltip handling.
- Updated Button.ts to only assign a default tooltip if the button is icon-only.
- Extended tests in Button.html and Cypress specs to validate the new tooltip behavior.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
packages/main/test/pages/Button.html | Added buttons with varied configurations to test tooltip behavior. |
packages/main/src/ButtonTemplate.tsx | Removed the unused showTooltip prop in the icon rendering. |
packages/main/src/Button.ts | Updated tooltip assignment logic to honor icon-only mode when applying the default tooltip. |
packages/main/cypress/specs/Button.cy.tsx | Added a test to ensure that a tooltip is not rendered when text is present. |
Comments suppressed due to low confidence (3)
packages/main/src/ButtonTemplate.tsx:50
- The removal of the showTooltip prop simplifies the template and aligns with the updated tooltip logic. Consider verifying that any related unused properties or code paths are also cleaned up to improve maintainability.
showTooltip={this.showIconTooltip}
packages/main/cypress/specs/Button.cy.tsx:242
- [nitpick] The test for ensuring no tooltip is displayed when text is present is good; however, consider using a more specific selector to target the intended button instance in case multiple buttons exist in the DOM.
cy.mount(<Button icon="home">Action</Button>);
packages/main/src/Button.ts:370
- The updated conditional assignment of 'buttonTitle' ensures that the default tooltip is only applied when the button is in icon-only mode. Please verify that cases where no tooltip is provided (and the button is not icon-only) correctly result in 'buttonTitle' remaining undefined.
const defaultTooltip = await this.getDefaultTooltip();
Issue:
ui5-button
even though there is a textin the
ui5-button
's slot.Solution:
ui5-button
is in icon-only modeaccording to the UX guidelines.
Fixes: #10824