fix(ui5-toolbar): fix ToolbarSelect flickering #11747
Conversation
There was a problem hiding this comment.
Pull Request Overview
Fixes a flickering issue in ToolbarSelect by adjusting item measurement and CSS application when the select overflows its toolbar.
- Scopes popover item width and alignment only when the select is overflowed
- Introduces a rendering flag to delay width measurement until after initial render
- Updates toolbar logic to skip items still rendering and adds an overflow behavior test
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ToolbarSelect.css | Changed .ui5-tb-popover-item rules to apply only under is-overflowed and set width to 100% |
| ToolbarSelect.ts | Conditional width style: unset when overflowed |
| ToolbarItem.ts | Added _isRendering flag and onAfterRendering hook |
| Toolbar.ts | Updated getItemWidth to ignore items while _isRendering |
| Toolbar.cy.tsx | Introduced a Cypress test for overflow scenario (uses it.only) |
Comments suppressed due to low confidence (1)
packages/main/cypress/specs/Toolbar.cy.tsx:410
- The use of
it.onlywill cause all other tests to be skipped. Remove.onlyto ensure the full test suite runs.
it.only("Should handle toolbar-select with width larger than the toolbar", async () => {
| @@ -107,6 +109,10 @@ class ToolbarItem extends UI5Element { | |||
| }, | |||
| }; | |||
| } | |||
|
|
|||
| onAfterRendering(): void { | |||
There was a problem hiding this comment.
Override of onAfterRendering does not call super.onAfterRendering(), which may skip essential base-class rendering logic. Consider adding super.onAfterRendering() at the start.
| onAfterRendering(): void { | |
| onAfterRendering(): void { | |
| super.onAfterRendering(); |
Copilot uses AI. Check for mistakes.
| @@ -60,6 +60,8 @@ class ToolbarItem extends UI5Element { | |||
| @property({ type: Boolean }) | |||
| isOverflowed: boolean = false; | |||
|
|
|||
| _isRendering = true; | |||
There was a problem hiding this comment.
The _isRendering flag is declared as a public property. It should be marked private or protected to indicate internal use and prevent external access.
| _isRendering = true; | |
| private _isRendering = true; |
Copilot uses AI. Check for mistakes.
| @@ -498,7 +498,7 @@ class Toolbar extends UI5Element { | |||
|
|
|||
| let itemWidth = 0; | |||
|
|
|||
| if (renderedItem && renderedItem.offsetWidth) { | |||
| if (renderedItem && !renderedItem.classList.contains("ui5-tb-popover-item") && renderedItem.offsetWidth && item._isRendering === false) { | |||
There was a problem hiding this comment.
This measurement condition is duplicated in getItemWidth and the initial width calculation. Consider extracting it into a helper function or constant to avoid duplication and hard-coded class names.
Copilot uses AI. Check for mistakes.
No description provided.