-
Notifications
You must be signed in to change notification settings - Fork 80
feat(checkbox): add component tokens #10221
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 4 commits
7d9d84f
b8eefa1
5907007
ba442c6
01ddeee
bbbfebc
e42b8ab
70d8589
6e129f5
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 |
---|---|---|
|
@@ -4,16 +4,31 @@ | |
* These properties can be overridden using the component's tag as selector. | ||
* | ||
* @prop --calcite-checkbox-size: Specifies the component's height and width. | ||
* @prop --calcite-checkbox-box-shadow: Specifies the component's color. | ||
* @prop --calcite-checkbox-box-shadow-hover: Specifies the component's color when hovered. | ||
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.
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. Just sanity check here on our patterns as it has changed a few times - this is preferable to calcite-checkbox:hover { --calcite-checkbox-shadow: "whatever" }, correct? 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 renamed this one to 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 believe so, but I'll let @alisonailea confirm. This approach is more consistent and intuitive than the previous one. It's also not mentioned as an example in the latest Component Tokens wiki. 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. Agreed. |
||
* @prop --calcite-checkbox-color: Specifies the component's font color. | ||
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. Can we revisit this description? It is used to set the @alisonailea Do we need to specify 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. we could use -checkbox-icon-color but I think -checkbox-color makes sense here but I'm open to either 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'll leave it as 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. Let’s use 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 updated the token to |
||
*/ | ||
|
||
:host([scale="s"]) { | ||
--calcite-checkbox-size: theme("spacing.3"); | ||
.check-svg, | ||
.toggle { | ||
inline-size: var(--calcite-checkbox-size, theme("spacing.3")); | ||
block-size: var(--calcite-checkbox-size, theme("spacing.3")); | ||
} | ||
} | ||
:host([scale="m"]) { | ||
--calcite-checkbox-size: theme("fontSize.n1"); | ||
.check-svg, | ||
.toggle { | ||
inline-size: var(--calcite-checkbox-size, theme("fontSize.n1")); | ||
block-size: var(--calcite-checkbox-size, theme("fontSize.n1")); | ||
} | ||
} | ||
:host([scale="l"]) { | ||
--calcite-checkbox-size: theme("spacing.4"); | ||
.check-svg, | ||
.toggle { | ||
inline-size: var(--calcite-checkbox-size, theme("spacing.4")); | ||
block-size: var(--calcite-checkbox-size, theme("spacing.4")); | ||
} | ||
} | ||
|
||
:host { | ||
|
@@ -23,12 +38,6 @@ | |
select-none; | ||
-webkit-tap-highlight-color: transparent; | ||
|
||
.check-svg, | ||
.toggle { | ||
inline-size: var(--calcite-checkbox-size); | ||
block-size: var(--calcite-checkbox-size); | ||
} | ||
|
||
.check-svg { | ||
@apply bg-foreground-1 | ||
pointer-events-none | ||
|
@@ -39,8 +48,8 @@ | |
stroke-current | ||
stroke-1 | ||
transition-default; | ||
box-shadow: inset 0 0 0 1px var(--calcite-color-border-input); | ||
color: theme("backgroundColor.background"); | ||
box-shadow: inset 0 0 0 1px var(--calcite-checkbox-box-shadow, var(--calcite-color-border-input)); | ||
color: var(--calcite-checkbox-color, theme("backgroundColor.background")); | ||
} | ||
} | ||
|
||
|
@@ -63,7 +72,7 @@ | |
:host([hovered]) .toggle, | ||
:host .toggle:hover { | ||
.check-svg { | ||
box-shadow: inset 0 0 0 2px var(--calcite-color-brand); | ||
box-shadow: inset 0 0 0 2px var(--calcite-checkbox-box-shadow-hover, var(--calcite-color-brand)); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,10 @@ | ||
import { html } from "../../support/formatting"; | ||
|
||
export const checkboxTokens = { | ||
calciteCheckboxSize: "", | ||
calciteCheckboxColor: "", | ||
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. add |
||
}; | ||
|
||
export const checkbox = html`<label> | ||
<calcite-checkbox indeterminate></calcite-checkbox> | ||
Initially indeterminate and unchecked | ||
|
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.
Upldate this to
--calcite-checkbox-box-shadow-color
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 thought we were using
shadow
, notbox-shadow
here? https://github.com/Esri/calcite-design-system/wiki/component-tokens#3-review-component-for-tokenizable-stylesThere 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.
Good catch, I totally missed this. The preferred naming for the CSS properties is parenthesized in the linked doc above, right? E.g.,
* border-radius (corner-radius)
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 renamed this one to
--calcite-checkbox-shadow-color
, as suggested in the doc above.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 realize it's basically being used as a border - but that begs the question.. Shouldn't this be a separate
--calcite-checkbox-border-color
(obfuscating the implementation detail of it using box-shadow from the user) - and then a separatecalcite-checkbox-shadow
css prop that defaults toshadow-none
to allow for theming?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.
@macandcheese Right again. I thought these new props were used for a drop shadow, but I overlooked how they were being applied.
@aPreciado88 The shadow props will need to be updated. The
themed
helper handles this and another case here. You can use this as a reference to confirm if we’re dealing with a border prop or not. LMK if you have any questions.Sorry for the hassle, y'all!
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 updated the tokens to
--calcite-checkbox-border-color
and--calcite-checkbox-border-color-hover
.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 I'm sorry. I agree with @macandcheese and @jcfranco