Focus: Remove default z-index
#2501
Conversation
✅ Deploy Preview for boosted ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
# Conflicts: # site/content/docs/5.3/migration.md
There was a problem hiding this comment.
I think I understand what you did, that is removing the default z-index: 5 everywhere added via the mixin focus-visible (with default value of $focus-visible-zindex as first parameter), and then add it again only inside our components that need it: accordion, buttons, navs, pagination, form-check.
I couldn't test it locally, and so I couldn't go really deeply inside your changes, and I have some questions:
- Can you explain why these components (and not the others) had to be updated?
- Why did you modify display and float properties in star rating?
- Why did you change the active state of list-group?
- Why did you point to button-group preview whereas there is no change in code?
- Can you explain for which problem/property you had to change the scss imports order?
Thanks :-)
Maybe the migration guide could be clarified too.
scss/_nav.scss
Outdated
| @@ -34,12 +34,18 @@ | |||
| border: 0; | |||
| @include transition($nav-link-transition); | |||
|
|
|||
| // Boosted mod: no focus | |||
| // Boosted mod: handle focus differently focus | |||
There was a problem hiding this comment.
"focus" word at the end to remove
scss/forms/_star-rating.scss
Outdated
| @@ -9,6 +9,7 @@ | |||
| --#{$prefix}star-rating-checked-icon: #{$form-star-rating-checked-icon}; | |||
| --#{$prefix}star-rating-unchecked-icon: #{$form-star-rating-unchecked-icon}; | |||
|
|
|||
| display: flex; | |||
There was a problem hiding this comment.
Both the display: flex and the removal of float: left changes were needed otherwise, the focus wasn't handled correctly. Here is the rendering without the changes:
Edit: Rethinking about this, it could even be display: inline-flex
| @@ -142,11 +142,6 @@ | |||
| &[data-focus-visible-added] { | |||
| outline-offset: subtract(-$focus-visible-outer-width, var(--#{$prefix}list-group-border-width)); | |||
| box-shadow: inset 0 0 0 add($focus-visible-inner-width, $focus-visible-outer-width) var(--#{$prefix}focus-visible-inner-color); | |||
|
|
|||
There was a problem hiding this comment.
This could be done separately if needed, if I remember well it was only an oversight from #1467.
site/content/docs/5.3/migration.md
Outdated
| ### Core | ||
|
|
||
| - **Colors** | ||
| - <span class="badge text-bg-warning">Warning</span> The dark mode red color hexadecimal value has been updated from `#f66` to `#ff4d4f` after a change in the design specifications to enhance the contrast for a better accessibility. This modification should be transparent for you except if you were using an hardcoded hexadecimal value directly in your websites. | ||
|
|
||
| ### CSS and Sass variables | ||
|
|
||
| - <span class="badge text-bg-danger">Breaking</span> A parameter from the `focus-visible()` mixin has been removed. You don't need the `z-index` parameter anymore. Please reflect these modifications inside your website. |
There was a problem hiding this comment.
Maybe explain this has an impact only if the project uses the mixin focus-visible() explicitly.
Proposition:
Breaking A parameter from the focus-visible() mixin has been removed. If you use the focus-visible() mixin in you scss files with a z-index as first parameter, please adapt your code.
There was a problem hiding this comment.
Yeah, I tried something based on your proposition in c37ccf4.
|
Based on the questions asked in the code review by @hannahiss, maybe the answers could be in the code somewhere. Otherwise, we'll break something in the future. It's probably not possible, but it would be even better if we couldn't gather this patch at the same place. I'm a bit afraid of maintaining that |
There is one more place to check: https://jsfiddle.net/gh/get/library/pure/googlemaps/js-samples/tree/master/dist/samples/add-map/jsfiddle with an addition to do in the
These components were updated because their behavior on focus wasn't trivial and might be breaking for people using it. The other were not because the focus wasn't broken.
I think it was because of the proximity of the components (the focus should go over the other buttons).
I can't remember exactly why but it seems to be related with the button groups. I think it was to avoid more CSS rules. I can maybe undo this change and rethink about a way of doing the good thing.
I tried my best in c37ccf4.
TBH, I don't really know what's the good way of doing this here. I could maybe add a file with some rules like |
Note: Please transform
- [ ]into- (NA)in the description when things are not applicableRelated issues
Closes #2496
Description
I focused the whole documentation and I think we are fine regarding the library.
⚠️ Changed the import order causing some issue between button-group and forms (button-group
z-indexrule is as specific as the forms:focusone)Tested on the reduced test case and it works fine again
Motivation & Context
Some elements outside the library were broken due to
z-indexproperty. See #2496. So we limit thez-indexonto library elements.Types of change
Live previews
Checklist
Contribution
Accessibility
Design
Development
Documentation
Checklist (for Core Team only)
After the merge