-
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
Tabs: Try a simpler tab focus style, alt #46276
Conversation
Size Change: -6.23 kB (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
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 testing well for me 💯
I just want to clear up the z-index stuff, because there are some anti-patterns here that I'd like to address before merging if the z-index styles are really necessary.
And if you could add a brief changelog for the TabPanel component, that would be great.
// See: https://github.com/WordPress/gutenberg/pull/9793 | ||
&:focus:not(:disabled) { | ||
position: relative; | ||
z-index: z-index(".edit-post-sidebar__panel-tab.is-active"); |
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 don't quite understand the purpose of all the z-index styles. Could you explain the intent?
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'm actually not sure — I know it was necessary at one point, but I think the tabs have been refactored enough that it might not be needed anymore. When I tried removing it just now, it worked fine without it. I'll try and remove it and see if that helps.
Co-authored-by: Lena Morita <[email protected]>
Co-authored-by: Lena Morita <[email protected]>
Thanks so much for the review, and for the suggestions. I've removed the z-index, and updated the changelog. I also noticed that the site editor had its own copy of the tab styles, so I updated them there as well. One thing I noted, and could use your advice on, is that the separate navigation screen code is apparently still in the codebase, and uses the old styles. I don't think this code is meant to be used anymore, so I didn't update it. @Mamaduka do you have any insights on what to best do here? Without updating that code, we can't remove the z-index layer from base-styles, this one: |
Thanks for the ping, @jasmussen. Can you give me a link to the navigation screen code in question? |
Sorry I should've included that: https://github.com/WordPress/gutenberg/blob/trunk/packages/edit-navigation/src/components/sidebar/style.scss#L57 In fact most of the tab code there is again just a verbatim copy of tab code being updated in this PR. It'd be nice to find a way to do more code reuse in the future, so it doesn't drift. Speaking of, I'll look into the widgets screen, I can't recall if it has tabs 🙈 |
I actually went ahead and updated both the widget and navigation screen tabs. It's unfortunate there's so much code duplication, but it doesn't seem like there's great value in holding on to the z-index layer if just obsolete code uses it. Let me know what you think and I can revert. |
Thanks, Joen. I saw the latest commit, so I guess that resolves the The Navigation Screen is no longer maintained (#43620), and I believe there's no way to access it via UI. So it's probably okay to break styles there. |
Thanks for the sanity check. I wasn't able to check, but the CSS I pushed shouldn't actually break anything, it's just that no-one will see it. But I had to make a change, because the code still gets compiled, and if it references a missing SCSS variable it breaks. |
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.
Looks good! 🚢 Thanks for the z-index clean up.
We'll be tackling the code duplication soon (#43414).
I'll trust these reviews then, thank you! |
Nice improvement! I'd suggest to move also the transparent outline used to support Windows High Contrast Mode from the button to the pseudo element, otherwise it's cut-off because one of the ancestors uses Worth reminding in Windows High Contrast Mode the CSS box-shadows aren't visible. The outline is. |
I'll do that in a followup when I get a moment. |
Thanks for working on this one! |
What?
Alternative to #46030. Explores a simpler tab focus style that isn't as heavy as it currently is.
Before:
After:
Testing Instructions + Testing Instructions for Keyboard
Navigate through tabs in the inspector and inserter and observe the simpler focus style. Be sure to test also the vertical orientation: https://wordpress.github.io/gutenberg/?path=/story/components-tabpanel--default&args=orientation:vertical — focus style should be the same.