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.
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 first draft of Popup (Editor's Draft) #355
Add first draft of Popup (Editor's Draft) #355
Changes from 1 commit
f5221f6
3211369
6d2dc00
66673dd
c76a3d7
3cd2129
a3a0e81
7dfbd62
610846d
a4de836
a000775
6e2fa36
14defbf
252119e
ecb4964
ccd9b75
dd86e42
5fc5041
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
defer to the
delegatesfocus
spec definitionThere 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.
See https://github.com/openui/open-ui/pull/355/files#r652867167
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.
So the spec for "Get the focusable area" should be used here, I think. And actually, I think it'd be a lot better if we folded this algorithm (focusing steps for
<popup>
and even<dialog>
) back into the "Get the focusable area" spec. It'd be a lot cleaner, I think.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.
Done in ecb4964! I've created a new section of this document at the bottom for text that should land in the focusable area algorithm.
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.
nit: I prefer for if/else type algos to have expectation in sub-bullet and be indented in. This is similar to how the code will normally look and IMO is easier to read and follow. This may not be how WHATWG does it however (having the result be another bullet).
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.
This is, unfortunately, how WHATWG typically writes if/else algorithms. Step N is "if X, do Y", and step N+1 is "otherwise, do Z".
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.
+1, I elected to follow WHATWG style here. Inviting HTML editors to weigh in on whether they'd be amenable to sub-bullets or if we should follow house style in order to land this text with minimal changes.
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.
What is this "returning" from?
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.
returning from the algorithm.
Here, I'm not sure about WHATWG conventions. This is an if/elseif/else block, as step 4 is the "otherwise" for the embedded "if" in step 3. That, I haven't exactly seen.
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.
Any changes requested, e.g. melding 4 into step 3?