chore(tokens): updated components to use semantic spacer tokens#6507
chore(tokens): updated components to use semantic spacer tokens#6507mcoker merged 18 commits intopatternfly:v6from
Conversation
|
Preview: https://patternfly-pr-6507.surge.sh A11y report: https://patternfly-pr-6507-a11y.surge.sh |
0589ad1 to
6871fb9
Compare
There was a problem hiding this comment.
Awesome! As mentioned offline, I just had questions around
- the multiplication/division of the tokens get large/small variations
- the use of the tokens within the layouts to create space between things like icon/text and other gaps/margins
30c1de4 to
95805d9
Compare
95805d9 to
07e5341
Compare
4e95c85 to
add995b
Compare
add995b to
98859f5
Compare
There was a problem hiding this comment.
Left some comments! Most are what we reviewed together. Also this PR has changed in scope since we first added tokens, and is a lot of stuff to keep track of in a single PR. I'm good with it if you want to keep it in this PR, or separate into multiple PRs, or get this to a point to merge with the intent to follow up in separate PRs... however you'd like to handle it 👍
| --#{$text-input-group}__utilities--child--MarginInlineStart: var(--pf-t--global--spacer--xs); | ||
| --#{$text-input-group}__utilities--c-button--PaddingInlineEnd: var(--pf-t--global--spacer--xs); | ||
| --#{$text-input-group}__utilities--c-button--PaddingInlineStart: var(--pf-t--global--spacer--xs); | ||
| --#{$text-input-group}__utilities--child--MarginInlineStart: calc(var(--pf-t--global--spacer--control--horizontal--plain) / 2); |
There was a problem hiding this comment.
I don't know if we have a spacer for this. --pf-t--global--spacer--gap--action-to-action--plain mayyy work, since I think the intention is we'll have plain actions here, but we have other things, too. IMO that might be alright, but we could err on the safe side and just continue to use --pf-t--global--spacer--xs. @lboehling wdyt? This is the gap used for the buttons and badge inside of the text input group on the right in this example https://patternfly-react-v6.surge.sh/components/search-input#match-with-navigable-options. Technically, I think you can put any kind of thing there you want.
There was a problem hiding this comment.
Reverted to t-shirt size for now
| --#{$tree-view}__node-toggle--PaddingBlockStart: var(--pf-t--global--spacer--control--vertical--default); | ||
| --#{$tree-view}__node-toggle--PaddingInlineEnd: var(--pf-t--global--spacer--control--horizontal--default); | ||
| --#{$tree-view}__node-toggle--PaddingBlockEnd: var(--pf-t--global--spacer--control--vertical--default); | ||
| --#{$tree-view}__node-toggle--PaddingInlineStart: var(--pf-t--global--spacer--control--horizontal--default); |
There was a problem hiding this comment.
I don't know that this needs to use a semantic spacer? My guess is the size of this box for the toggle icon just needs to match the tree view node padding. Similar to the table, if someone were to theme the semantic control/action spacer, they probably don't intend for this box to change shape, and if it did, it could cause the node box to extend out of the parent node.
98859f5 to
e618af8
Compare
28ca469 to
296fe48
Compare
There was a problem hiding this comment.
There are a few changes to color tokens - are these intended in this PR? Just checking...
|
|
||
| // Compact/Spacious spacers - TO BE DELETED ONCE FIGMA IS UPDATED | ||
| --pf-t--global--spacer--action--vertical--spacious: var(--pf-t--global--spacer--md); | ||
| --pf-t--global--spacer--action--horizontal--spacious: var(--pf-t--global--spacer--xl); | ||
| --pf-t--global--spacer--action--vertical--compact: var(--pf-t--global--spacer--xs); | ||
| --pf-t--global--spacer--action--horizontal--compact: var(--pf-t--global--spacer--md); | ||
| --pf-t--global--spacer--action--horizontal--plain--compact: var(--pf-t--global--spacer--xs); | ||
| --pf-t--global--spacer--control--vertical--compact: var(--pf-t--global--spacer--xs); | ||
| --pf-t--global--spacer--control--horizontal--compact: var(--pf-t--global--spacer--md); | ||
| --pf-t--global--spacer--control--horizontal--spacious: var(--pf-t--global--spacer--lg); |
There was a problem hiding this comment.
Looks like these have made it to the big time!
296fe48 to
5e28a07
Compare
|
@srambach removed a couple of lines related to Badge color/background that Evan removed in another PR, so should no longer be needed here. The other color token should be correct, assuming you mean the @lboehling The only token I didn't remove from the local token file was |
| position: absolute; | ||
| inset-block-start: 50%; | ||
| inset-inline-start: var(--#{$text-input-group}__icon--InsetInlineStart); | ||
| font-size: var(--#{$text-input-group}__icon--FontSize); |
There was a problem hiding this comment.
This is a non-spacer change so if that's just a rebase thing, it's ok.
5e28a07 to
90246ba
Compare
568f249 to
d845e52
Compare
| // Main | ||
| --#{$text-input-group}__main--first-child--not--text-input--MarginInlineStart: var(--pf-t--global--spacer--xs); | ||
| --#{$text-input-group}__main--m-icon__text-input--PaddingInlineStart: var(--pf-t--global--spacer--xl); | ||
| --#{$text-input-group}__main--first-child--not--text-input--MarginInlineStart: calc(var(--pf-t--global--spacer--control--horizontal--plain) / 2); |
There was a problem hiding this comment.
padding-block-start: var(--#{$text-input-group}__main--first-child--not--text-input--PaddingBlockStart); and padding-block-end: var(--#{$text-input-group}__main--first-child--not--text-input--PaddingBlockEnd); on line 158-9 are undefined - should it they defined here?
There was a problem hiding this comment.
Originally those padding-inlines were margins for the label group, but since we have the block on line ~163 for the padding on label group, removing the padding properties on lines 158-9.
d845e52 to
dcfe185
Compare
|
Pushed a fix for the action--horizontal--plain token Here's the visual regression report https://drive.google.com/file/d/16seRrrSUEh3vW8tkMNGkHEuhPP_5lxrR/view?usp=sharing |
bf264b9 to
6b296fb
Compare
|
Fixed a couple of bugs reported by @srambach. The wizard close button position shifted, and the toggle button/icon in the complex form demo with field groups shifted. Both can be verified as fixed in the shiny new visual regression report. |
There was a problem hiding this comment.
LGTM!!! Thanks for working through this @thatblindgeye, expanding and helping build out the needed semantic spacers, and keeping the PR up to date all this time. You rock!!
Opened a follow up for text input group -#6930
|
🎉 This PR is included in version 6.0.0-alpha.204 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #6292 , #6465