Repository navigation
Theme: Add density support for surface padding #73215
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
Changes from 8 commits
e07057d
db45b28
f2c0bf1
a2c55d2
32279f9
fbcb7b0
0d37ed4
79291f9
25ac161
4fe0f86
94848d3
c9c8985
80cbbea
56672d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -558,27 +558,43 @@ | |
| }, | ||
| "description": "Base dimension unit" | ||
| }, | ||
| "Dimension/Semantic/padding-surface-x-small": { | ||
| "Dimension/Semantic/padding-surface-2xs": { | ||
| "value": { | ||
| ".": "8px" | ||
| ".": "4px", | ||
| "compact": "4px", | ||
| "comfortable": "8px" | ||
| }, | ||
| "description": "2x extra small spacing for surfaces" | ||
| }, | ||
| "Dimension/Semantic/padding-surface-xs": { | ||
| "value": { | ||
| ".": "8px", | ||
| "compact": "4px", | ||
| "comfortable": "12px" | ||
|
aduth marked this conversation as resolved.
Resolved
|
||
| }, | ||
| "description": "Extra small spacing for surfaces" | ||
| }, | ||
| "Dimension/Semantic/padding-surface-small": { | ||
| "Dimension/Semantic/padding-surface-sm": { | ||
| "value": { | ||
| ".": "12px" | ||
| ".": "16px", | ||
| "compact": "12px", | ||
| "comfortable": "20px" | ||
| }, | ||
| "description": "Small spacing for surfaces" | ||
| }, | ||
| "Dimension/Semantic/padding-surface-medium": { | ||
| "Dimension/Semantic/padding-surface-md": { | ||
| "value": { | ||
| ".": "16px" | ||
| ".": "24px", | ||
| "compact": "20px", | ||
| "comfortable": "32px" | ||
| }, | ||
| "description": "Medium spacing for surfaces" | ||
| }, | ||
| "Dimension/Semantic/padding-surface-large": { | ||
| "Dimension/Semantic/padding-surface-lg": { | ||
| "value": { | ||
| ".": "24px" | ||
| ".": "32px", | ||
| "compact": "24px", | ||
| "comfortable": "40px" | ||
| }, | ||
| "description": "Large spacing for surfaces" | ||
| }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we'll probably need tokens for 32px and 48px padding. 32px is currently used in Dialogs and 48px in DataViews. I guess that also means we'll need a new primitive so that 48px can jump to 56px in comfortable density. |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,11 @@ export interface ThemeProviderSettings { | |
| */ | ||
| bg?: string; | ||
| }; | ||
|
|
||
| /** | ||
| * The density of the theme. | ||
| */ | ||
| density?: 'compact' | 'comfortable'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should there be a middle-ground default, 'balanced' or similar?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I understand there is a default, but it's not named. Feel free to resolve :D
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the absence of an override is considered the default. We could have something more explicit, though I think this mirrors nicely to the way it works in the tokens, which is that the densities are "extensions" (i.e. overrides) to some base value.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should at least mention in the prop description that it's going to use some other default density as the default. As it looks to a consumer now, we might as well just forgotten to document whether the default is |
||
| } | ||
|
|
||
| export interface ThemeProviderProps extends ThemeProviderSettings { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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 don't have a strong opinion but I'm wondering if a naming convention like
xs, sm, md, lg, xlwould be better.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 we could still change it at this point if we think it'd be an improvement. For context, these are largely carried over from existing conventions like what exist in base-styles (example)
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 the shorter names are a bit easier to interpret when the scale grows. IE
4xsis slightly more intuitive than4-x-small. It also makes the tokens names shorter, and a bit easier to scan in a list. So I think I lean in that direction, but only slightly. Keen to hear other thoughts cc @mattmiklic.