Skip to content
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

refactor(ui5-menu): adjust menu and sub-menu creation #8367

Merged
merged 5 commits into from
Mar 25, 2024

Conversation

unazko
Copy link
Contributor

@unazko unazko commented Feb 28, 2024

  • The ui5-menu elements used for sub-menus are created only once and are being reused afterwards. They are no longer destroyed on close. This contributes to lowering the count of the slow DOM manipulation operations.

  • There is now no differentiation between mobile and desktop device in regards to the display mechanism. In both cases we rely on the template to do the job as the components used for composition like ui5-list and ui5-responsive-popover do comply with the device.

Fixes: #7767
Fixes: #7423
Fixes: #6761

- The ui5-menu elements used for sub-menus are created only once
and are being reused afterwards. They are no longer destroyed on close.
This contributes to lowering the count of the slow DOM manipulation operations.

- There is now no differentiation between mobile and desktop
device in regards to the display mechanism. In both cases
we rely on the template to do the job as the components used
for composition like ui5-list and ui5-responsive-popover do
comply with the device.

Fixes: SAP#7767
Fixes: SAP#7423
Fixes: SAP#6761
Related to: SAP#7391
Copy link
Contributor

@tsanislavgatev tsanislavgatev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:
Now when hovering with mouse on a menu with busy indicator, the focus goes to busy indicator.
We have arrow back in main menu on mobile view.

@unazko unazko requested a review from tsanislavgatev March 1, 2024 08:28
@NHristov-sap NHristov-sap added this to the enu_opening_issues milestone Mar 1, 2024
@NHristov-sap
Copy link
Contributor

One from me: on mobile view, there is a focus on disabled items, please check.

@unazko
Copy link
Contributor Author

unazko commented Mar 1, 2024

One from me: on mobile view, there is a focus on disabled items, please check.

The focus appears on an actual mobile device on item press. The same behavior was also reproducible before this change
and also this behavior actually comes from the ui5-list component. It could be reproduced there if the ui5-item-click
event gets prevented at application side.

@unazko
Copy link
Contributor Author

unazko commented Mar 1, 2024

Two things: Now when hovering with mouse on a menu with busy indicator, the focus goes to busy indicator. We have arrow back in main menu on mobile view.

Those two cases are now handled properly.

Copy link
Contributor

@tsanislavgatev tsanislavgatev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not able to select the first item.(Space, Enter or Mouse click). Is that something with the sample or an issue?

@unazko
Copy link
Contributor Author

unazko commented Mar 5, 2024

I am not able to select the first item.(Space, Enter or Mouse click). Is that something with the sample or an issue?

The default behavior for the menu item is prevented at application level. I've improved the tooltip and menu item texts, in order to give detail about the item selection as we've discussed.

@unazko unazko requested a review from tsanislavgatev March 5, 2024 08:23
@unazko unazko marked this pull request as ready for review March 5, 2024 11:30
@ilhan007 ilhan007 removed this from the enu_opening_issues milestone Mar 22, 2024
@unazko unazko merged commit b668a50 into SAP:main Mar 25, 2024
9 checks passed
ilhan007 pushed a commit that referenced this pull request Mar 29, 2024
- The ui5-menu elements used for sub-menus are created only once
and are being reused afterwards. They are no longer destroyed on close.
This contributes to lowering the count of the slow DOM manipulation operations.

- There is now no differentiation between mobile and desktop
device in regards to the display mechanism. In both cases
we rely on the template to do the job as the components used
for composition like ui5-list and ui5-responsive-popover do
comply with the device.

Fixes: #7767
Fixes: #7423
Fixes: #6761
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants