-
Notifications
You must be signed in to change notification settings - Fork 76
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(stepper, stepper-item): add component tokens #8906
feat(stepper, stepper-item): add component tokens #8906
Conversation
@@ -6,27 +8,31 @@ | |||
* @prop --calcite-stepper-action-background-color: defines the background color of an action sub-component inside the component. | |||
* @prop --calcite-stepper-action-background-color-hover: defines the background color of an action sub-component when hovered or focused. | |||
* @prop --calcite-stepper-action-background-color-active: defines the background color of an action sub-component when active. | |||
* @prop --calcite-stepper-step-bar-fill-color : Specifies fill color for step-bar when `layout="horizontal-single"`. |
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.
Q: Should this tokens be specific ? The reason being that calcite-stepper-item-border-color
token allows users to specify the border-color at stepper-item
level. Adding the above tokens helps users customize border-color at stepper
level when layout ="horizontal-single"
. Wonder if its ideal to specify them for single use case of layout or should this be replaced with calcite-stepper-item-border-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.
One way is to get rid of border-top for stepper-item and let user configure step-bar irrespective of layout.
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 even in horizontal-single
I'd expect the step bar color to match the item border color of the standard horizontal. Is that not how this works currently?
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.
currently it matches, but if the user wants to customize the colors using tokens they need to setup twice once for horizontal & vertical and again for horizontal-single. Both horizontal and vertical layout depend on stepper-item color where as horizontal-single depends on step-bar component to show border like.
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.
Sorry for the confusion, i was under the impression that responsive mode is a default setting when the container size decreases but we made a change recently where user needs to specify the layout horizontal-single
for single stepper view. Make sense to have these separate set of tokens . Ideally one set of tokens would be preferable but the way component is designed can be tricky .
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 still don’t know if we need two - can we use the same nomenclature for both? The user still places the steps in the DOM in horizontal-single right? I might be missing something too, lol.
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.
With the current way the component is design we end up with having one set of tokens to customize border for vertical & horizontal and other set for horizonatal-layout. In horizontal-single layout case the border isn't the border of stepper-item but a functional step-bar which is not attached to any stepper-item. Using step-bar for every layout would be ideal but requires a refactor.
User can customize the border color as following:
// covers the use-case of layout="horizontal-single"
calcite-stepper {
--calcite-stepper-step-bar-fill-color: green;
--calcite-stepper-step-bar-active-fill-color: #e9f6ff;
--calcite-stepper-step-bar-complete-fill-color: #280274;
--calcite-stepper-step-bar-error-fill-color: #fe7a36;
}
//covers use-case of layout="horizontal" or layout="vertical"
calcite-stepper-item {
--calcite-stepper-item-border-color: green;
}
calcite-stepper-item[error] {
--calcite-stepper-item-border-color: #fe7a36;
}
calcite-stepper-item[complete] {
--calcite-stepper-item-border-color: #280274;
}
calcite-stepper-item[selected] {
--calcite-stepper-item-border-color: #e9f6ff;
}
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 I refactor might be worth it, in the long-term at least. Not sure if this is the time to do that or not. I think this situation may have come about do to limitations of how we build stuff in Figma. That version needs a separate step-bar for horizontal-single
out of necessity, but I don't see why we'd need to mimic that in the real version.
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.
Agree on the refactor, ideally these colors needs to be customized once irrespective of the layout. Configuring the colors twice is redundant and can be very confusing.
The reason for adding step-bar
in the real version is to display the borders which are attached to stepper-item. In horizontal-single
layout mode where only one item is viewed, borders of each stepper-item is unavailable. To resolve this situation we came up with the step-bar component. In the initial designs we are displaying only one border in single-view related to currently viewing stepper-item. In further discussion, felt it is better to show the step-bar equivalent to no of stepper-items renders so that user has an idea of how many steps are present.
i think its better to use step-bar for all the layout and drop the border maybe since horizontal-single
layout cannot rely on border? (considering stepper-item's are not used outside of stepper) . Open to suggestions.
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.
How much would a refactor take? This seems like a lot of extra tokens.
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.
Stepper and Stepper Item can not exist without one another so let's try to share tokens/styles where possible
The following should be set only in Stepper and can be simplified/inherited by stepper-item
--calcite-internal-stepper-item-spacing-unit-s = --calcite-stepper-item-spacing-unit-s
--calcite-internal-stepper-action-inline-size = --calcite-internal-stepper-action-inline-size
--calcite-internal-step-bar-gap = stepper-item:host { margin-inline-end }
As to the refactor, there are a few other components that had a bit of a refactor to reduce component tokens. How much of a refactor are we talking about here? I think it might be worth it.
padding-block: 0.25rem; | ||
inline-size: 100%; | ||
padding-block: var(--calcite-spacing-xxs); | ||
inline-size: var(--calcite-container-size-content-fluid); | ||
} | ||
|
||
@mixin stepBar { |
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.
There's no reason for this to be a mixin. Either these styles need to be moved to a step-bar scss file or just remove the mixin wrapper.
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.
The reason for mixin is to isolate the step-bar related styles and improve readability as suggested.
packages/calcite-components/src/components/stepper/stepper.scss
Outdated
Show resolved
Hide resolved
--calcite-internal-stepper-action-inline-size: theme("spacing.8"); | ||
--calcite-internal-step-bar-gap: theme("spacing.1"); | ||
--calcite-internal-stepper-action-inline-size: var(--calcite-spacing-xxxl); | ||
--calcite-internal-step-bar-gap: var(--calcite-spacing-xxs); |
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.
Leave all rem based styles as REM for now.
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.
Curious if there is a downside of using semantic tokens for REM values.
packages/calcite-components/src/components/stepper/stepper.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/stepper-item/stepper-item.scss
Show resolved
Hide resolved
packages/calcite-components/src/components/stepper-item/stepper-item.scss
Show resolved
Hide resolved
packages/calcite-components/src/components/stepper-item/stepper-item.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/stepper-item/stepper-item.scss
Outdated
Show resolved
Hide resolved
@@ -6,27 +8,31 @@ | |||
* @prop --calcite-stepper-action-background-color: defines the background color of an action sub-component inside the component. | |||
* @prop --calcite-stepper-action-background-color-hover: defines the background color of an action sub-component when hovered or focused. | |||
* @prop --calcite-stepper-action-background-color-active: defines the background color of an action sub-component when active. | |||
* @prop --calcite-stepper-step-bar-fill-color : Specifies fill color for step-bar when `layout="horizontal-single"`. |
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.
How much would a refactor take? This seems like a lot of extra tokens.
packages/calcite-components/src/components/stepper/stepper.scss
Outdated
Show resolved
Hide resolved
@apply text-n1h; | ||
--calcite-stepper-item-spacing-unit-s: var(--calcite-spacing-xxs); | ||
--calcite-stepper-item-spacing-unit-m: var(--calcite-spacing-md); | ||
--calcite-stepper-item-spacing-unit-l: var(--calcite-spacing-xl); |
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-stepper-item-spacing-unit-[s,m,l] need to be internal.
- We should use "space" instead of "spacing"
- Keep using the
theme("spacing
REM values for now - simplify the scales
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.
example
:host {
margin-inline-end: var(--calcite-internal-stepper-item-space-unit-s);
}
.container {
font-size: var(--calcite-internal-stepper-item-font-size);
line-height: var(--calcite-internal-stepper-item-line-height);
}
.stepper-item-description {
font-size: var(--calcite-internal-stepper-item-description-font-size);
line-height: var(--calcite-internal-stepper-item-description-line-height);
}
:host([scale="s"]) {
.container {
--calcite-internal-stepper-item-space-unit-s: theme("spacing.1");
--calcite-internal-stepper-item-space-unit-m: theme("spacing.3");
--calcite-internal-stepper-item-space-unit-l: theme("spacing.4");
--calcite-internal-stepper-action-inline-size: theme("spacing.8");
--calcite-internal-stepper-item-font-size: var(--calcite-font-size--1);
--calcite-internal-stepper-item-line-height: var(--calcite-font-size--1);
--calcite-internal-stepper-item-description-font-size: var(--calcite-font-size--2);
--calcite-internal-stepper-item-description-line-height: var(--calcite-font-size--1);
}
}
--calcite-internal-stepper-action-inline-size: var(--calcite-spacing-xxxl); | ||
|
||
font-size: var(--calcite-font-size); | ||
line-height: var(--calcite-font-line-height-fixed-base); | ||
margin-inline-end: theme("margin.1"); |
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.
this is the same value as --calcite-stepper-item-spacing-unit-s
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.
Not sure why we required separate ones for margin and spacing in first place.
Related Issue: #7180
Summary
Adds tokens for
stepper
andstepper-item
components.calcite-stepper-item
tokens:calcite-stepper
tokens: