Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(block): add component tokens #8790
feat(block): add component tokens #8790
Changes from 1 commit
a2f67bc
f90cad4
cac8446
fcbb0d1
23d4af2
58e6b7e
5da8c32
9bf724e
8dbffd7
862704d
7cf2c4e
0a15fd7
e606e2a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
padding
here is more descriptive - space is a bit ambiguous. Maybecalcite-block-content-padding
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think spacing makes more sense. That way we could internally swap between padding/margin/whatever if need be. Do we want to be specific with padding or margin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should decide on space vs spacing too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, even
content-space
could make sense I guess. Butspace
could be anything - space between text, slots, etc. In Modal we do use--calcite-modal-content-padding
. In the future "content areas" could be ::parts with defaults.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have spacing vars like
--calcite-spacing-xxs
so it only makes sense to have component spacing vars like--calcite-block-spacing-
IMO.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Panel, we added
calcite-panel-footer-space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calcite-block-content-space
works for me. We can update elsewhere we currently usepadding
to follow this. As long as it has a context and isn't justcomponent-space
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew this seemed familiar. We had a similar discussion in #8629 (comment) and landed on
--calcite-tab-content-block-padding
.SGTM, but I'd like for @alisonailea to chime on this too. If we needed separate props later on for block/inline values, would we introduce
--calcite-component-space-<block|inline>
? If so, we could add an example of this in our component token guidelines.We decided on
space
to align withsize
(vs.sizing
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that our design system is not necessarily web specific, we need to use platform agnostic naming conventions.
space
is the preferred terminology here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For posterity, if we need custom space props that only apply horizontally or vertically, we can use something like
--calcite-component-<part>-space-<x|y>
(see #8852 (comment)).