feat(nav): added tokens, new design spec#6033
Conversation
|
Preview: https://patternfly-pr-6033.surge.sh A11y report: https://patternfly-pr-6033-a11y.surge.sh |
ad945cd to
2c2e3dd
Compare
|
In pages, you can see that we've got a large spacer on the inline-start of the page sidebar AND on the navigation. I think the padding should be removed from the page sidebar, WDYT? |
@srambach Yep, I agree 👍 |
|
Agree with the comments above on the spacer |
There was a problem hiding this comment.
Couple of minor things, but looks really good! Maybe you can walk us through the handlebars helpers sometime?
| {{#if nav-link--text}} | ||
| {{> nav-link}} | ||
| {{/if}} | ||
| {{#if @partial-block}} |
There was a problem hiding this comment.
Just curious about the reason to put the partial-block inside a conditional
There was a problem hiding this comment.
Great question, the conditional allows for additional content in the nav__item. Without the conditional, partial content is required, which then requires a closing tag
{{#> nav-item nav-link--text='my-content'}}{{/nav-item}}
vs
{{> nav-item nav-link--text='my-content'}})
|
this is looking great 🎉, thank you @mattnolting!! a few of comments --
|
Done. Thanks @srambach 👍
Done.
Done.
@lboehling I think it would be great to see both examples. The example you're referring to is controlled by
Should Should Something like this:
|
274a6eb to
95e394b
Compare
39cba3e to
2100343
Compare
2100343 to
73b0fad
Compare
40177a7 to
ce99f2b
Compare
e4317c2 to
b0d0bb6
Compare
There was a problem hiding this comment.
LGTM, the only thing I'd like to look at before merging is the button enhancement to have its own class to mirror/flip its icon, since that's adding a feature to the react button. IIRC, last we spoke about it we wanted to try and use <Icon shouldMirrorRTL> anywhere there isn't a core class we can target to apply a mixin and you can pass the icon to something.
There was a problem hiding this comment.
Oh forgot to leave this comment - the new button vars you've introduced are not initially defined in the button component. Not sure if that's on purpose or not, but at a minimum the border-radius vars need to be added back since the button has lost its rounded corners https://patternfly-pr-6033.surge.sh/components/button
bdf71b3 to
b0ff109
Compare
There was a problem hiding this comment.
Looks like a change to the inline button padding snuck through - was that intentional? Also it would be great to fix the display property, though it works as it is now, too.
| display: var(--#{$button}--Display, initial); | ||
| flex: var(--#{$button}--Flex, initial); | ||
| align-items: var(--#{$button}--AlignItems, initial); | ||
| justify-content: var(--#{$button}--JustifyContent, initial); |
There was a problem hiding this comment.
"initial" for the value of display is technically "inline", although it doesn't appear to be break anything, looks like the browser won't allow it to be a span. Should probably be "revert" though if you want to use whatever the browser default is.
Same for the others, but less important since they're ignored until you make it a flex element.
| --#{$button}--PaddingRight: var(--pf-t--global--spacer--lg); | ||
| --#{$button}--PaddingRight: var(--pf-t--global--spacer--md); | ||
| --#{$button}--PaddingBottom: var(--pf-t--global--spacer--sm); | ||
| --#{$button}--PaddingLeft: var(--pf-t--global--spacer--lg); | ||
| --#{$button}--PaddingLeft: var(--pf-t--global--spacer--md); |
There was a problem hiding this comment.
Not sure if this was intentional, but the design for button uses large spacers for the inline padding
| @@ -304,6 +303,10 @@ | |||
|
|
|||
| padding-inline: var(--#{$nav}__scroll-button--button--InlinePadding); | |||
There was a problem hiding this comment.
I'd say this should probably modify --#{$button}--PaddingLeft and --#{$button}--PaddingRight. We pretty much always style each side of padding/margin, unless we're updating all sides (usually to "0"). If we do want to change that and style inline/block as a single declaration/value, this would need to be --PaddingInline. We used to style the inline/block sides with like padding: var(--component--PaddingY) var(--component--PaddingX); but stopped, thought I don't remember why?
| --#{$button}--start-end--Radius--BorderRadius: 0; | ||
| --#{$button}--end-start--Radius--BorderRadius: var(--#{$nav}__scroll-button--BorderRadius); | ||
| --#{$button}--end-end--Radius--BorderRadius: 0; | ||
| button { |
There was a problem hiding this comment.
Just curious - why are you targeting button instead of the button component class? Here, L298, and L353.
|
🎉 This PR is included in version 6.0.0-alpha.50 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #6000