chore(menu): support delivering submenus via directives #4226
Conversation
This file contains 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
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
Tachometer resultsChromeaction-bar permalinkbasic-test
action-menu permalinktest-basic
test-directive permalink
test-lazy permalink
test-open-close-directive permalink
test-open-close permalink
combobox permalinkbasic-test
light-dom-test permalink
menu permalinktest-basic
overlay permalinkbasic-test
directive-test permalink
element-test permalink
lazy-test permalink
picker permalinkbasic-test
popover permalinktest-basic
split-button permalinkbasic-test
tooltip permalinktest-basic
test-directive permalink
test-element permalink
test-lazy permalink
truncated permalinkbasic-test
Firefoxaction-bar permalinkbasic-test
action-menu permalinktest-basic
test-directive permalink
test-lazy permalink
test-open-close-directive permalink
test-open-close permalink
combobox permalinkbasic-test
light-dom-test permalink
menu permalinktest-basic
overlay permalinkbasic-test
directive-test permalink
element-test permalink
lazy-test permalink
picker permalinkbasic-test
popover permalinktest-basic
split-button permalinkbasic-test
tooltip permalinktest-basic
test-directive permalink
test-element permalink
test-lazy permalink
truncated permalinkbasic-test
|
|
The submenu flickers a little when I open it with my mouse. 🤔 Does that happen for you? I'm assuming "no"... Screen.Recording.2024-03-27.at.11.49.08.mov |
Westbrook
force-pushed
the
submenu-directive
branch
from
cce127b
to
7b7028f
Compare
Westbrook
force-pushed
the
submenu-directive
branch
from
7b7028f
to
cc1cc80
Compare
Westbrook
commented
Mar 28, 2024
Comment on lines
+543
to
+545
| if (this.lastRequestSlottableState === this.open) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Great catch on the stutter, @najikahalsema, we should use broken browsers more often? This will prevent duplicate slottable-request events from being dispatched with the same data, which appears to be what was causing what you captured, even if it was really difficult to run into normally.
Rajdeepc
reviewed
Mar 28, 2024
| @@ -39,6 +39,7 @@ import checkmarkStyles from '@spectrum-web-components/icon/src/spectrum-icon-che | |||
| import type { Menu } from './Menu.js'; | |||
| import { MutationController } from '@lit-labs/observers/mutation-controller.js'; | |||
| import type { Overlay } from '@spectrum-web-components/overlay'; | |||
| import { SlottableRequestEvent } from '@spectrum-web-components/overlay/src/slottable-request-event.js'; | |||
There was a problem hiding this comment.
I am not sure whether we have expanded the readme to include this usage but we can definately add some context now on this for the consumers in a good document on how and when to use it?
There was a problem hiding this comment.
This unlocks the last bit of scope for the initial test interaction with the PS team. Once that test cycle is complete, the last piece of confirmation before documentation/"official release" will be confirming how the directives work in partnership with requests for content in slots other than the default slot AND THEN I will start writing documentation with all available speed. One part of that documentation will also be internal outlining path for future use of this form of "enhancement testing process".
Rajdeepc
reviewed
Mar 28, 2024
| flatten: true, | ||
| }); | ||
| this.hasSubmenu = !!assignedElements.length; | ||
| })[0] as HTMLElement; |
There was a problem hiding this comment.
Can we keep a null pointer check here?
There was a problem hiding this comment.
The return value for assignedElements() is always an array, even if it's empty, which means that accessing the 0 index will either get an Element or undefined, which are the two values that we expect here. Is there an alternative form of protection that you'd like to see a null pointer check guard us from?
hunterloftis
approved these changes
Mar 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Expand use of
slottable-requestbased laziness to support submenus.How has this been tested?
Types of changes
Checklist