-
Notifications
You must be signed in to change notification settings - Fork 218
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
[Popup] Add declarative invocation and move event bubbling to an appendix #459
[Popup] Add declarative invocation and move event bubbling to an appendix #459
Conversation
Co-authored-by: Ionel Popescu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this!
I just have some nits and a comment on the hidden
attribute.
Popup/explainer.md
Outdated
e.stopPropagation() | ||
e.currentTarget.dispatchEvent(new CommandEvent(e.currentTarget.id)) | ||
} | ||
Note: for many `popup`s, the element which invokes the `popup` and the element the `popup` is anchored to will be one in the same. However, there are cases where the author may want to anchor to a child/parent of the element which invoked the popup. Similiarly, there are cases (such as this teaching UI example) where no such invoking element exists. Therefore, we do not propose collapsing invocation and anchoring responsibilities to one attribute, as they are distinct responsibilities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with not collapsing both responsibilities to one attribute. But for convenience, does it make sense that the popup
attribute should create some sort of "automatic" anchor
attribute on the <popup>
, if no such attribute actually exists? I.e.
<button popup=p></button>
<popup id=p></popup>
in this case, the anchor element for #p
would be <button>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this seems like a natural way to go and something we discussed. I think the potential issues would be: what if there's more than one element with a popup
attribute pointing to the given popup
? Which would be used as the anchor? This open issue might have bearing on resolution as well: https://github.com/MicrosoftEdge/MSEdgeExplainers/pull/459/files#diff-66cb270cf5b767927a4ab78042b4ea5e1e52c2c2c372f5e3d3d3b192787175d4R321
I think it's better to call this out though so per your comment added this note: 3251149
efe374d
to
3e7994e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! :)
3d17d94
to
2e93cab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes here, LGTM!
No description provided.