-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Content structure: Fix Semantics #15474
Conversation
I don't think that the outline should be disabled. See #15479 which was merged this week by @jasmussen. |
We need to indicate somehow what has focus, yes, especially when it's set like this. We have some options on visuals, though, and in the pr linked I've leveraged one style. |
Left some comments for things that need to be changed. Re: the redundant |
5388055
to
0c1d652
Compare
I believe I have addressed all the feedback now. I also noticed we disable the eslint rule and I used the same thing, including the explanation. |
Hey @marekhrabe, this one will need to be rebased for the tests to pass. There was fix merged to master to resolve an issue introduced by the WordPress 5.2 release. |
A build restart should be sufficient in most cases, as far as I've seen. I'll give it a try. |
It might be who mentioned it. I think it's still a valid proposal to introduce a hihger-level component for Lists which would ensure that all good practices are applied behind the scenes. In general, I don't think that you should be ever using raw HTML tag in Gutenberg outside of |
No specifics come to mind, but it's been a recurring point across a number of separate issues that the abstraction of a component affords us a useful tool to guarantee consistency and best-practices across the entire application. A |
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.
Code wise everything looks good.
Whether we go with List or just updating eslint to recommend the explicit role for lists, I believe both should be done separately on a whole repo scope.
Yes, this should be tackled separately. We should simply blacklist li
from using in code outside of List
component and inside every save
method of the block. Well, it's not that simple, but I think the best way to move forward in general to let ESLint handle it.
@afercia, I did some testing but I would appreciate your sanity check to confirm that everything looks good. I hope I will gain more confidence over time and I won't have to ping you so often :D
@gziolo looks good to me 👍
|
Yes, it's kind of expected with how popovers are implemented. Isn't there an open issue to bring a similar behavior to popover like to modal, where all other DOM elements outside of popover are marked with some aria attributes which improves that experience? |
Not sure there's a specific issue. It was discussed previously and yes maybe we should come to a decision on how to treat the various popovers. |
Thanks @marekhrabe for working on this issue. |
Description
Fixes #15290
I hope I understood the proposed solution correctly and I've updated the markup from divs into ul>li. This came with an additional need to reset the margin to match the original styling.
After updating to
<ul>
, the outline showed up after opening the popover as the focus moves there:The outline of
<div>
is disabled globally in wp-admin/css/common.min.css. There is no such rule for<ul>
but to 100% match the original behavior and looks, I have disabled the outline for this specific usage. Let me know if I should keep the outline enabled so the result looks like the screenshot above.Title was updated to use
<h2>
. No visual regression there - it looks the same.How has this been tested?
Screenshots