chore(Various): updated CSS gap vars to use semantic tokens (part 1)#6957
chore(Various): updated CSS gap vars to use semantic tokens (part 1)#6957mcoker merged 5 commits intopatternfly:mainfrom
Conversation
|
Preview: https://patternfly-pr-6957.surge.sh A11y report: https://patternfly-pr-6957-a11y.surge.sh |
There was a problem hiding this comment.
A few questions based on inspection of what the variable/token controls. I'll leave the decisions about whether spacing increases/decreases to design :-)
I'd love to move the local tokens out to figma, though each is only used once. Will there be more? Also, are there others to create where "no applicable spacer" exists?
| --#{$drawer}__panel--BackgroundColor: var(--pf-t--global--background--color--primary--default); | ||
| --#{$drawer}__panel--m-secondary--BackgroundColor: var(--pf-t--global--background--color--secondary--default); | ||
| --#{$drawer}__panel--RowGap: var(--pf-t--global--spacer--sm); | ||
| --#{$drawer}__panel--RowGap: var(--pf-t--global--spacer--gap--text-to-element--default); |
There was a problem hiding this comment.
This feels like a --pf-t--global--spacer--gap--group--vertical maybe? Looks like it's between the parts of a drawer?
There was a problem hiding this comment.
Revert back to spacer sm
| --#{$dual-list-selector}__controls--PaddingInlineEnd: var(--pf-t--global--spacer--md); | ||
| --#{$dual-list-selector}__controls--PaddingInlineStart: var(--pf-t--global--spacer--md); | ||
| --#{$dual-list-selector}__controls--Gap: var(--pf-t--global--spacer--sm); | ||
| --#{$dual-list-selector}__controls--Gap: var(--pf-t--global--spacer--gap--group--vertical); |
There was a problem hiding this comment.
WDYT about --pf-t--global--spacer--gap--control-to-control--default here? It would decrease the spacing but seems more semantic.
There was a problem hiding this comment.
Update to --pf-t--global--spacer--gap--action-to-action--plain
| // Section | ||
| --#{$form}__section--MarginBlockStart: var(--pf-t--global--spacer--xl); | ||
| --#{$form}__section--Gap: var(--pf-t--global--spacer--md); | ||
| --#{$form}__section--Gap: var(--pf-t--global--spacer--gap--group--horizontal); |
There was a problem hiding this comment.
Should this be --pf-t--global--spacer--gap--group--vertical? It would reduce the space. OR maybe it's --pf-t--global--spacer--gap--group-to-group--vertical even though it's within a form section.
There was a problem hiding this comment.
Updated to use the form--gridgap var per discussion in meeting
| --#{$content}--dl--ColumnGap: var(--pf-t--global--spacer--gap--group-to-group--horizontal); | ||
| --#{$content}--dl--RowGap: var(--pf-t--global--spacer--gap--group-to-group--vertical); // Increases the current gap |
There was a problem hiding this comment.
Met with @lboehling today and we looked at the content <dl> and description-list component (for another PR - https://github.com/patternfly/patternfly/pull/6953/files) and the row-gap here is good but the column gap should be --pf-t--global--spacer--gap--group--horizontal
| --#{$content}--dl--ColumnGap: var(--pf-t--global--spacer--gap--group-to-group--horizontal); | |
| --#{$content}--dl--RowGap: var(--pf-t--global--spacer--gap--group-to-group--vertical); // Increases the current gap | |
| --#{$content}--dl--ColumnGap: var(--pf-t--global--spacer--gap--group--horizontal); | |
| --#{$content}--dl--RowGap: var(--pf-t--global--spacer--gap--group-to-group--vertical); |
There was a problem hiding this comment.
On rebase, omit this update
| --#{$description-list}--RowGap: var(--pf-t--global--spacer--gap--group-to-group--vertical); | ||
| --#{$description-list}--ColumnGap: var(--pf-t--global--spacer--gap--group-to-group--horizontal); // Increases current gap |
There was a problem hiding this comment.
I think these are both good 👍
| --#{$description-list}__group--RowGap: var(--pf-t--global--spacer--gap--group--vertical); | ||
| --#{$description-list}__group--ColumnGap: var(--pf-t--global--spacer--gap--group--horizontal); // Increases current Gap |
f890d3b to
a4b713e
Compare
There was a problem hiding this comment.
@thatblindgeye I met with @lboehling and did a group review and had these suggestions. Let me know if you'd like to go ahead and make the changes (and as long as everything works, we can just merge) or if you'd prefer merge this as-is and we can follow up post-v6 - either way is cool with me!
|
|
||
| @include pf-root($check) { | ||
| --#{$check}--GridGap: var(--pf-t--global--spacer--sm) var(--pf-t--global--spacer--sm); | ||
| --#{$check}--GridGap: var(--pf-t--global--spacer--gap--text-to-element--default) var(--pf-t--global--spacer--gap--text-to-element--default); |
There was a problem hiding this comment.
Confirmed with @lboehling we can use the gap--group--vertical for the row gap, which matches description list and form.
| --#{$check}--GridGap: var(--pf-t--global--spacer--gap--text-to-element--default) var(--pf-t--global--spacer--gap--text-to-element--default); | |
| --#{$check}--GridGap: var(--pf-t--global--spacer--gap--group--vertical) var(--pf-t--global--spacer--gap--text-to-element--default); |
| --#{$clipboard-copy}--m-inline--BackgroundColor: var(--pf-t--global--background--color--secondary--default); | ||
| --#{$clipboard-copy}__actions--Gap: var(--pf-t--global--spacer--sm); | ||
| --#{$clipboard-copy}__actions--Gap: var(--pf-t--global--spacer--gap--action-to-action--default); | ||
| --#{$clipboard-copy}__actions--MarginInlineStart: var(--pf-t--global--spacer--xs); |
There was a problem hiding this comment.
Confirmed with @lboehling we can use the new var you created --pf-t--global--spacer--gap--text-to-element--compact here
| --#{$clipboard-copy}__actions--MarginInlineStart: var(--pf-t--global--spacer--xs); | |
| --#{$clipboard-copy}__a CDA3 ctions--MarginInlineStart: var(--pf-t--global--spacer--gap--text-to-element--compact); |
There was a problem hiding this comment.
Just to confirm, did you mean update teh MarginInlineStart token to the text-to-element spacer, or update the actions--Gap?
EDIT: Of course I just saw your comment below 😆 Different question then I suppose, does it make sense to use a gap token for the MarginInline/Block properties? I think we had maybe concluded/assumed that a "gap" spacer would be fitting for "inside" spacing, while a non-gap spacer would be fitting for the "outside" spacing (around the box more or less). If that's accurate then the gap spacer would probably work here, just want to confirm if that is still the line of thought for the gap spacers.
| --#{$clipboard-copy}--m-inline--PaddingInlineEnd: var(--pf-t--global--spacer--xs); | ||
| --#{$clipboard-copy}--m-inline--BackgroundColor: var(--pf-t--global--background--color--secondary--default); | ||
| --#{$clipboard-copy}__actions--Gap: var(--pf-t--global--spacer--sm); | ||
| --#{$clipboard-copy}__actions--Gap: var(--pf-t--global--spacer--gap--action-to-action--default); |
There was a problem hiding this comment.
| --#{$clipboard-copy}__actions--Gap: var(--pf-t--global--spacer--gap--action-to-action--default); | |
| --#{$clipboard-copy}__actions--Gap: var(--pf-t--global--spacer--gap--action-to-action--plain); |
| // controls | ||
| --#{$code-editor}__controls--PaddingInlineStart: var(--pf-t--global--spacer--sm); | ||
| --#{$code-editor}__controls--Gap: var(--pf-t--global--spacer--xs); | ||
| --#{$code-editor}__controls--Gap: var(--pf-t--global--spacer--gap--control-to-control--default); |
There was a problem hiding this comment.
| --#{$code-editor}__controls--Gap: var(--pf-t--global--spacer--gap--control-to-control--default); | |
| --#{$code-editor}__controls--Gap: var(--pf-t--global--spacer--gap--action-to-action--plain); |
|
|
||
| // 3E1D Lists | ||
| --#{$content}--list--Gap: var(--pf-t--global--spacer--sm); | ||
| --#{$content}--list--Gap: var(--pf-t--global--spacer--gap--group--vertical); |
There was a problem hiding this comment.
Chatting with @lboehling we may look to add some content specific tokens in the future, so to be consistent with other content elements (description list excluded) let's update this to be the generic t-shirt spacer for now.
| --#{$content}--list--Gap: var(--pf-t--global--spacer--gap--group--vertical); | |
| --#{$content}--list--Gap: var(--pf-t--global--spacer--sm); |
| - CDA3 -#{$form}__group--Gap: var(--pf-t--global--spacer--gap--group--vertical); | ||
| --#{$form}__group--m-action--MarginBlockStart: var(--pf-t--global--spacer--xl); | ||
| --#{$form}--m-horizontal__group-label--md--GridColumnWidth: #{pf-size-prem(150px)}; | ||
| --#{$form}--m-horizontal__group-label--md--GridColumnGap: var(--pf-t--global--spacer--md); |
There was a problem hiding this comment.
| --#{$form}--m-horizontal__group-label--md--GridColumnGap: var(--pf-t--global--spacer--md); | |
| --#{$form}--m-horizontal__group-label--md--GridColumnGap: var(--pf-t--global--spacer--gap--group--horizontal); |
|
|
||
| // Form group label info | ||
| --#{$form}__group-label--m-info--Gap: var(--pf-t--global--spacer--sm); | ||
| --#{$form}__group-label--m-info--Gap: var(--pf-t--global--spacer--gap--text-to-element--default); |
There was a problem hiding this comment.
| --#{$form}__group-label--m-info--Gap: var(--pf-t--global--spacer--gap--text-to-element--default); | |
| --#{$form}__group-label--m-info--Gap: var(--pf-t--global--spacer--gap--group--horizontal); |
| --#{$form}__group-label--m-info--Gap: var(--pf-t--global--spacer--gap--text-to-element--default); | ||
| --#{$form}__group-label-info--FontSize: var(--pf-t--global--font--size--body--sm); | ||
| --#{$form}--m-horizontal__group-label--m-info--Gap: var(--pf-t--global--spacer--sm); | ||
| --#{$form}--m-horizontal__group-label--m-info--Gap: var(--pf-t--global--spacer--gap--text-to-element--default); |
There was a problem hiding this comment.
| --#{$form}--m-horizontal__group-label--m-info--Gap: var(--pf-t--global--spacer--gap--text-to-element--default); | |
| --#{$form}--m-horizontal__group-label--m-info--Gap: var(--pf-t--global--spacer--gap--group--vertical); |
|
|
||
| // Form control utilities | ||
| --#{$form-control}__utilities--Gap: var(--pf-t--global--spacer--sm); | ||
| --#{$form-control}__utilities--Gap: var(--pf-t--global--spacer--gap--text-to-element--default); |
There was a problem hiding this comment.
| --#{$form-control}__utilities--Gap: var(--pf-t--global--spacer--gap--text-to-element--default); | |
| --#{$form-control}__utilities--Gap: var(--pf-t--global--spacer--gap--group--horizontal); |
|
|
||
| @include pf-root($hint) { | ||
| --#{$hint}--GridRowGap: var(--pf-t--global--spacer--sm); | ||
| --#{$hint}--GridRowGap: var(--pf-t--global--spacer--gap--group--vertical); |
There was a problem hiding this comment.
Per @lboehling this is a good candidate for a more content specific token, so we can use a generic t-shirt spacer for now.
| --#{$hint}--GridRowGap: var(--pf-t--global--spacer--gap--group--vertical); | |
| --#{$hint}--GridRowGap: var(--pf-t--global--spacer--sm); |
17017c1 to
29f78ed
Compare
| // controls | ||
| --#{$code-editor}__controls--PaddingInlineStart: var(--pf-t--global--spacer--sm); | ||
| --#{$code-editor}__controls--Gap: var(--pf-t--global--spacer--gap--control-to-control--plain); | ||
| --#{$code-editor}__controls--Gap: var(--pf-t--global--spacer--gap--action-to-action--plain); |
There was a problem hiding this comment.
Updated to match the suggestion in #6957 (comment). Looks like a typo, --pf-t--global--spacer--gap--control-to-control--plain isn't a valid token.
|
|
||
| // Group | ||
| --#{$form}__group--Gap: var(--pf-t--global--spacer--gap--group--horizontal); | ||
| --#{$form}__group--Gap: var(--pf-t--global--spacer--gap--group--vertical); |
There was a problem hiding this comment.
Not sure why this was updated to --horizontal. The default layout for __group is a stack/vertical layout. Just reverting to what was there when we left some feedback around this block - #6957 (comment)
| --#{$form}__group--m-action--MarginBlockStart: var(--pf-t--global--spacer--xl); | ||
| --#{$form}--m-horizontal__group-label--md--GridColumnWidth: #{pf-size-prem(150px)}; | ||
| --#{$form}--m-horizontal__group-label--md--GridColumnGap: var(--pf-t--global--spacer--md); | ||
| --#{$form}--m-horizontal__group-label--md--GridColumnGap: var(--pf-t--global--spacer--gap--group--horizontal); |
There was a problem hiding this comment.
Applying the change suggested from #6957 (comment). I'm guessing this was overlooked, or possibly confused with the update to --#{$form}__group--Gap that I backed out in this commit.
| --#{$form}__group-label--m-info--Gap: var(--pf-t--global--spacer--gap--horizontal); | ||
| --#{$form}__group-label-info--FontSize: var(--pf-t--global--font--size--body--sm); | ||
| --#{$form}--m-horizontal__group-label--m-info--Gap: var(--pf-t--global--spacer--gap--vertical); | ||
| --#{$form}--m-horizontal__group-label--m-info--Gap: var(--pf-t--global--spacer--gap--group--vertical); |
There was a problem hiding this comment.
Applying suggestion from #6957 (comment). Looks like a typo, group-- was left off (--pf-t--global--spacer--gap--vertical isn't a valid token)
|
|
||
| // Form control utilities | ||
| --#{$form-control}__utilities--Gap: var(--pf-t--global--spacer--gap--horizontal); | ||
| --#{$form-control}__utilities--Gap: var(--pf-t--global--spacer--gap--group--horizontal); |
There was a problem hiding this comment.
Applying suggestion from #6957 (comment). Looks like a typo, --pf-t--global--spacer--gap--horizontal isn't a valid token.
|
🎉 This PR is included in version 6.0.0-prerelease.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Towards #6715
Decided to put this PR up that only touches some files rather than a huge PR, partially because there's some questions to maybe iron out. I added a few tokens to the local file that I'm wondering if we should add to Figma, and I left some comments about replacement tokens being a different gap than the current gap or CSS vars that didn't seem to have an applicable gap token (at least based on name).