refactor(cdk-experimental/menu): refactor MenuStack to ensure submenus are popped off and closed #20006
Conversation
googlebot
added
the
cla: yes
| * Pop off the top most MenuStackItem and emit it on the close observable. | ||
| * Pop off the top most MenuStackItem and emit it on the close observable if the stack is not | ||
| * empty. If `lastElement` is provided it will pop off until this element (inclusive). If | ||
| * `lastElement` is not in the stack, it will pop off the topmost element only. |
There was a problem hiding this comment.
This seems like somewhat odd behavior for such a function -- if anything, I would expect it to keep popping until the end of the stack if it doesn't encounter the specified lastElement.
It's not wrong per se, but it feels counterintuitive. Is there a way to achieve the intended overall behavior without relying on this behavior?
There was a problem hiding this comment.
If you're referring to what happens when lastElement is not provided that was just a fallback behaviour decision for which under normal circumstances it shouldn't be encountered. I figured that in the edge case where that may occur it would be better to pop off rather than do nothing. I'm flexible to either approach though.
| * @param focusNext the event to emit on the `empty` observable if the method call resulted in an | ||
| * empty stack. Does not emit if the stack was initially empty. | ||
| */ | ||
| closeExclusive(lastElement?: MenuStackItem, focusNext?: FocusNext) { |
There was a problem hiding this comment.
Hmm, now there are two methods that do the same thing if you don't provide a parameter, but behave differently if you do. Are you sure these two methods definitely need the first parameter to be optional? Also, can you achieve the effect you need with a pair of methods that don't overlap? (Eg. 'closeExclusive() and closeLatest()'.) That way, you might be able to avoid the slightly redundant method implementations.
andy9775
force-pushed
the
cdk-menu-stack-close-till
branch
from
84b2561 to
752ffa9
Compare
| if (menuStackItem) { | ||
| this._close.next(menuStackItem); | ||
| if (this._elements.length === 0) { | ||
| closeInclusive(lastItem: MenuStackItem, focusNext?: FocusNext) { |
There was a problem hiding this comment.
How about just close for this method (with the closing of all its sub-menus implied) and then either closeChildrenOf or closeSubMenusOf for the other method?
There was a problem hiding this comment.
Sounds good. There is already a close observable but I renamed it to closed along with empty to emptied. LMK
| expect(menus.length).toBe(3); | ||
|
|
||
| menuStack.close.subscribe(item => { | ||
| expect(item).toEqual(menus.pop()!); |
There was a problem hiding this comment.
Why is this non-null assertion (!) necessary here?
There was a problem hiding this comment.
pop may return undefined and the expect call requires a MenuStackItem. I changed it around though so it's not necessary anymore though which addresses the below comment as well
| openAllMenus(); | ||
| expect(menus.length).toBe(3); | ||
|
|
||
| menuStack.close.subscribe(item => { |
There was a problem hiding this comment.
Having an expect in a subscribe is tricky because you're not guaranteed that the stream will actually emit.
|
|
||
| /** Return true if this stack is empty. */ | ||
| isEmpty() { | ||
| return this._elements.length === 0; |
There was a problem hiding this comment.
optionally could do:
| return this._elements.length === 0; | |
| return !!this._elements.length; |
andy9775
force-pushed
the
cdk-menu-stack-close-till
branch
2 times, most recently
from
ffe6f83 to
baeda3e
Compare
|
@teflonwaffles @jelbourn feedback should be addressed. |
…s are popped off and closed When closing a menu from the menu stack this ensures that any sub-menu's are popped off the stack and closed resulting in a stack which represents the state of the UI.
andy9775
force-pushed
the
cdk-menu-stack-close-till
branch
from
baeda3e to
c66cd11
Compare
jelbourn
added
lgtm
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
When closing a menu from the menu stack this ensures that any sub-menu's are popped off the stack
and closed resulting in a stack which represents the state of the UI.