[IMP] Menu: Delegate enabled to children items #7489
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
|
hokolomopo
reviewed
Nov 21, 2025
Comment on lines
+254
to
+256
| this.openingTimeOut.schedule(() => { | ||
| this.openSubMenu(menu, currentTarget); | ||
| }, TIMEOUT_DELAY); |
Contributor
There was a problem hiding this comment.
I find it a bit strange UX-wise that the menu opens while there is a cursor: not-allowed and no hover/effect background. I'd remove the cursor and add back the hover effect for disabled parent menus
src/components/menu/menu.ts
Outdated
Comment on lines
122
to
123
| if (children.length) { | ||
| const areChildrenEnabled = children.length |
Contributor
There was a problem hiding this comment.
you check children.length twice
Contributor
There was a problem hiding this comment.
can just write if(chidren.length) return children.some((child) => this.isEnabled(child));
| await simulateClick(".o-menu div[data-name='root']"); | ||
| expect(fixture.querySelector(".o-menu div[data-name='subMenu']")).toBeFalsy(); | ||
| }); | ||
| describe("'IsEnabled' is ignored on parent menu items", () => { |
Contributor
There was a problem hiding this comment.
nitpick: don't use quotes in test names, we cannot use jest debugger then
Currently, if a menu is evaluated as disabled, we disable it and prevent the possibility to see its children by hover. However, there are some issues with this: - the user is mislead because we keep showing the arrow that suggest the presence of children items - the user can simply not navigate through the submenus to see what kind of feature could be available - if a menu has several children some of which are readonlyAllowed, then the readonlyAllowed condition has to be set on the parent as well; the 'enabling' condition therefore becomes a combination set on both the parent and children - if a parent menu is computed as 'enabled' it will not be greyed out (suggests that it has useful children) even if the said children are not enabled. This revision changes the computation of 'enabled' state of item by looking at the full tree of children and applies the following logic; A parent menu is disabled if every leaf of the tree is disabled. A single 'enabled' leaf (that is a child without children of its own) will make all its parent enabled as well, up to the root. We also allow all submenus to be opened regardless of the state of the parent. The user will therefore have a visual hint that there are active submenus or not (if all children are disabled, so will be the parent) and they will also have the possibility to have a look at all children to explore the possibilities. Task: 5331717
rrahir
force-pushed
the
master-ref-menu-rar
branch
from
770b463 to
418f89c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, if a menu is evaluated as disabled, we disable it and prevent the possibility to see its children by hover. However, there are some issues with this:
This revision changes the computation of 'enabled' state of item by looking at the full tree of children and applies the following logic; A parent menu is disabled if every leaf of the tree is disabled. A single 'enabled' leaf (that is a child without children of its own) will make all its parent enabled as well, up to the root. We also allow all submenus to be opened regardless of the state of the parent.
The user will therefore have a visual hint that there are active submenus or not (if all children are disabled, so will be the parent) and they will also have the possibility to have a look at all children to explore the possibilities.
Task: 5331717
Description:
description of this task, what is implemented and why it is implemented that way.
Task: TASK_ID
review checklist