chore(Compass): replaced flex layout in CompassMainHeader#7984
Conversation
|
Preview: https://pf-pr-7984.surge.sh A11y report: https://pf-pr-7984-a11y.surge.sh |
| } | ||
|
|
||
| .#{$compass}__main-header { | ||
| & .#{$compass}__panel { |
There was a problem hiding this comment.
The & is redundant when using a nested element like this, though you'd want to target the direct descendent in case for some weird reason there were nested panel elements
| & .#{$compass}__panel { | |
| > .#{$compass}__panel { |
| .#{$compass}__main-header { | ||
| & .#{$compass}__panel { | ||
| display: flex; | ||
| flex-wrap: wrap; |
There was a problem hiding this comment.
I'm not sure if these should wrap. Typically with these types of elements, we don't wrap by default. It's up to the user to manage the toolbar size so it leaves room for the title. For card, we have a .pf-m-wrap modifier for the header. I looked at the existing demos and the flex layout wraps, but IMO that's because it's the default for the flex layout, not an intentional choice (not by me anyways).
There was a problem hiding this comment.
Can remove, this was just one of the properties from the Flex layout components I ported over to this.
| display: flex; | ||
| flex-wrap: wrap; | ||
| gap: var(--#{$compass}__main-header--RowGap) var(--#{$compass}__main-header--ColumnGap); | ||
| align-items: center; |
There was a problem hiding this comment.
If they don't wrap by default, we'd want to take this off and leave things top-aligned or baseline aligned or something like that, so that if the title text wraps, it wraps like the card. And because the actions should be taller than the title, the title would probably need align-self: center
|
|
||
| .#{$compass}__main-header-title { | ||
| flex-grow: 1; | ||
| max-width: 100%; |
There was a problem hiding this comment.
Curious why you needed max-width: 100%? Sometimes the contents of flex items overflow their container, but it's most often due to min-width: min-content as the default for a flex item, and you'd disable that with min-width: 0;.
There was a problem hiding this comment.
Similar to above, was one of the things applied with the Flex layout components being used that I ported to this. Wasn't sure exactly what properties we'd want on these new classes after getting rid of those Flex components
| } | ||
|
|
||
| .#{$compass}__main-header-toolbar { | ||
| max-width: 100%; |
There was a problem hiding this comment.
same comment about max-width: 100% as on the block above.
| .#{$compass}__main-header-title { | ||
| flex-grow: 1; | ||
| } |
There was a problem hiding this comment.
You can take this out of the nesting, it should only be used in this context. The lower the specificity the better in general.
|
🎉 This PR is included in version 6.5.0-prerelease.22 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Closes #7969