fix(Toolbar): Added hidden class to content element#6678
Conversation
|
Preview: https://patternfly-pr-6678.surge.sh A11y report: https://patternfly-pr-6678-a11y.surge.sh |
There was a problem hiding this comment.
Left a comment about what pf-v6-hidden-visible() does that hopefully clarifies a change needed to get this hooked up.
Also for the .history/* files showing up, that's the first I've seen of .history files making it into a PR but looks like it's in react's .gitignore, so feel free to add it to core's as part of this PR!
|
|
||
| // - Toolbar content - Toolbar group - Toolbar item - Toolbar group label container - Toolbar group label group | ||
| // - Toolbar content - Toolbar content section - Toolbar group - Toolbar item - Toolbar group label container - Toolbar group label group | ||
| .#{$toolbar}__content, |
There was a problem hiding this comment.
pf-v6-hidden-visible() generates .pf-m-hidden and .pf-m-visible classes for selectors its used with, and takes an argument for the value of the display property used to show the selector(s) that is used with .pf-m-visible. Here's some fake code to show what I mean:
.pf-c-component,
.pf-c-another-component {
@include pf-v6-hidden-visible(flex);
}
// compiles to this CSS
.pf-c-component.pf-m-hidden,
.pf-c-another-component.pf-m-hidden {
display: none;
}
.pf-c-component.pf-m-visible,
.pf-c-another-component.pf-m-visible {
display: flex; // "flex" is the argument passed to pf-v6-hidden-visible() above
}
All of the selectors in this block where .#{$toolbar}__content was added are flex layouts, so they can all be grouped this way. However, .#{$toolbar}__content is a grid layout, so it can't be used in this group. We can see that it is display: grid here
patternfly/src/patternfly/components/Toolbar/toolbar.scss
Lines 79 to 83 in 9f6d47c
So in short we just need to create a new block for .#{$toolbar}__content to use pf-v6-hidden-visible(). Something like
.#{$toolbar}__content {
@include pf-v6-hidden-visible(grid);
}
Then remove the display: grid declaration on .#{$toolbar}__content here, since pf-v6-hidden-visible() will set it
But we'll want to make sure .#{$toolbar} is still set to display: grid; and both .#{$toolbar} and .#{$toolbar}__content are set to position: relative.
There was a problem hiding this comment.
Looks good, just one small nit!
| .#{$toolbar}, | ||
| .#{$toolbar}__content { | ||
| position: relative; | ||
| display: grid; |
There was a problem hiding this comment.
Since this removes display: grid; from .#{$toolbar}, too, we'll need to add that back. You can probably just add it as the first .#{$toolbar} style here
patternfly/src/patternfly/components/Toolbar/toolbar.scss
Lines 88 to 89 in c7a6d8b
|
🎉 This PR is included in version 6.0.0-alpha.137 🎉 The release is available on: Your semantic-release bot 📦🚀 |
closes: #6627