Ship Overlay v2 #3456
Merged
Ship Overlay v2 #3456
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
Westbrook
requested review from
cuberoot,
beeduul,
benjamind,
hunterloftis,
joekukish,
spdev3000,
najikahalsema and
adixon-adobe
Tachometer resultsChromeaccordion permalink
action-bar permalink
action-button permalink
action-group permalink
action-menu permalink
asset permalink
avatar permalink
badge permalink
banner permalink
button-group permalink
button permalink
card permalink
checkbox permalink
coachmark permalink
color-area permalink
color-handle permalink
color-loupe permalink
color-slider permalink
color-wheel permalink
dialog permalink
divider permalink
dropzone permalink
field-group permalink
field-label permalink
grid permalink
help-text permalink
icon permalink
icons permalink
illustrated-message permalink
link permalink
menu permalink
meter permalink
number-field permalink
overlay permalink
picker-button permalink
picker permalink
popover permalink
progress-bar permalink
progress-circle permalink
quick-actions permalink
radio permalink
search permalink
sidenav permalink
slider permalink
split-button permalink
split-view permalink
swatch permalink
switch permalink
table permalink
tabs permalink
tags permalink
textfield permalink
thumbnail permalink
toast permalink
tooltip permalink
top-nav permalink
tray permalink
underlay permalink
Firefoxaccordion permalink
action-bar permalink
action-button permalink
action-group permalink
action-menu permalink
asset permalink
avatar permalink
badge permalink
banner permalink
button-group permalink
button permalink
card permalink
checkbox permalink
coachmark permalink
color-area permalink
color-handle permalink
color-loupe permalink
color-slider permalink
color-wheel permalink
dialog permalink
divider permalink
dropzone permalink
field-group permalink
field-label permalink
grid permalink
help-text permalink
icon permalink
icons permalink
illustrated-message permalink
link permalink
menu permalink
meter permalink
number-field permalink
overlay permalink
picker-button permalink
picker permalink
popover permalink
progress-bar permalink
progress-circle permalink
quick-actions permalink
radio permalink
search permalink
sidenav permalink
slider permalink
split-button permalink
split-view permalink
swatch permalink
switch permalink
table permalink
tabs permalink
tags permalink
textfield permalink
thumbnail permalink
toast permalink
tooltip permalink
top-nav permalink
tray permalink
underlay permalink
|
11 tasks
|
|
||
| ### trigger | ||
|
|
||
| The `trigger` option accepts an `HTMLElement` or a `VirtualTrigger` from which to position the Overlay. |
There was a problem hiding this comment.
Several of these options reference a trigger. Should we specify here the side-effects of not providing a trigger, with an example of why you'd use or not use one?
There was a problem hiding this comment.
Initially optional to support straight to <dialog> opening of Overlay.open((overlayElement: HTMLElement));.
Clarify what not having a trigger in other cases means...
|
Some early clarification questions as I start to review -
What kind of unexpected behavior?
What does this mean exactly? Edit: after our sync I think I know the gist of this. |
Westbrook
force-pushed
the
overlay-mixins
branch
from
52365b0 to
ef8c956
Compare
Westbrook
force-pushed
the
overlay-mixins
branch
from
ef8c956 to
56f2c01
Compare
Westbrook
force-pushed
the
overlay-mixins
branch
from
393c916 to
f064251
Compare
Westbrook
force-pushed
the
overlay-mixins
branch
from
bd0e3b0 to
fc87ce2
Compare
This was referenced Aug 21, 2023
Closed
[Bug]: sometimes sp-closed event is not received from sp-action-menu after closing the popover
#3117
Closed
1 task
charlessuh
reviewed
Sep 5, 2023
| }); | ||
| const content = this.contentSlot | ||
| .assignedNodes() | ||
| .map((node) => node.cloneNode(true)); |
There was a problem hiding this comment.
Isn't it possible for contentSlot or iconSlot to be undefined / null?
There was a problem hiding this comment.
In the frame before the first call to render(), sure there is a moment in which these are not yet defined. There's likely harder to define issues that arise in that moment than this, but we'd love to hear if you happen to have a use case where that arises! Then we can make sure it's covered in our unit test suite and resilient to any subsequent changes that may come to this part of the library.
There was a problem hiding this comment.
Thanks! I created #3626.
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
Update the Overlay API to leverage
<dialog>elements and thepopoverAPI as outlined in this discussion.To do:
active-overlayin the migration docssp-overlayEnterin Pickerdelayedin the imperative APIselectsis providedThere are a lot of changes herein. To help focus review, I've sequestered specific packages/functional areas into each of the commits in this branch:
Specific feature areas for deeper thought
[type="page"]Related issue(s)
openattribute #3184icon-onlyattribute #3398Motivation and context
Overlays are hard, we want to make it easier through the magic of browsers.
How has this been tested?
Rigorously, extensively, but still with the need of your help!
Types of changes
Checklist