-
Notifications
You must be signed in to change notification settings - Fork 3k
Add popover attribute #8221
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
Add popover attribute #8221
Conversation
I see that there are many build errors after build.sh says "Running conformance checker...", but when I run build.sh locally, it just stops after "Success!". How do I run the conformance checker when building locally? |
The conformance checker is at https://github.com/validator/validator and has a variety of installation options. You need to run it on the output |
Thanks, I was able to run it! It looks like the PR builds now |
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 managed to get a start on this review today... still more to go, but hopefully this helps.
Thanks for the review! I'm going to be out until monday so I'll try to get to it then |
Thanks for the review @domenic! I have addressed all your comments |
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.
Not an authoritative review; but some questions that popped up while quickly reading the PR.
@domenic mind taking another look? Anything else I can do to help move this forward? |
It's on my list, sorry! I had a week of TPAC then a week of vacation, so I'm currently down to 20 flagged work emails and 24 flagged GitHub emails :). |
Drive-by: element and attribute indices haven't been updated as far as I can tell. (FWIW, I plan on doing a more careful review.) |
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.
Lots of editorial issues; as always, if you can be sure to do an extra sweep to make sure you catch multiple instances, that saves a lot of time.
On normative issues, I think these are the biggest outstanding ones:
- show/hide naming (under discussion in Open UI I guess, not sure how that's going)
- Element vs. HTMLElement; i.e., behavior for unknown, SVG, and MathML elements
- Some confusion about spec text for some animation pausing stuff
- Light dismiss spec intercepting capture events on Document is quite unusual; @annevk's help there would be appreciated steering us in the right direction
Looks like we might rename them to popupshow and popuphide: openui/open-ui#607
@mfreed7 Why did you implement all of the popup behavior in Element instead of HTMLElement? What do you think the behavior should be for unknown, SVG, and MathML elements? |
My general logic has been "why not?". I.e. we had the same debate in the TAG about limiting this feature to only some element types. And if there's a good reason to limit it, great. But if the only reason is that we don't know what it'll be used for, I think web developers will figure that out. For example, why not a pop-up SVG? I could think of some use cases where an icon (a "like button" for example) is made with SVG and would like to be a pop-up. Side note: this is currently broken in Chromium, but pending this discussion, I'll fix that. Just a missing set of rules in the UA stylesheet. |
Feature proposal issue: #7785
Explainer: https://open-ui.org/components/popup.research.explainer
/browsers.html ( diff )
/dnd.html ( diff )
/dom.html ( diff )
/form-elements.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/input.html ( diff )
/interaction.html ( diff )
/interactive-elements.html ( diff )
/rendering.html ( diff )
/semantics-other.html ( diff )
/webappapis.html ( diff )
/popover.html ( diff )