-
Notifications
You must be signed in to change notification settings - Fork 77
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(popover): add component tokens #8642
feat(popover): add component tokens #8642
Conversation
packages/calcite-components/src/components/popover/popover.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/popover/popover.scss
Outdated
Show resolved
Hide resolved
…nent-tokens-for-popover
packages/calcite-components/src/components/popover/popover.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/popover/popover.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/popover/popover.scss
Outdated
Show resolved
Hide resolved
…nent-tokens-for-popover
…nent-tokens-for-popover
packages/calcite-components/src/components/popover/popover.scss
Outdated
Show resolved
Hide resolved
This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions. |
packages/calcite-components/src/components/popover/popover.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/popover/popover.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/popover/popover.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/popover/popover.scss
Outdated
Show resolved
Hide resolved
* @prop --calcite-popover-border-color: The border color of the component. | ||
* @prop --calcite-popover-corner-radius: The start end border radius of the component. | ||
* @prop --calcite-popover-text-color: The text color of the component. | ||
* @prop --calcite-popover-z-index: Sets the z-index value for the component. |
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.
My mistake here. We're keeping z-index
an -internal-
variable for now.
* @prop --calcite-popover-corner-radius: The start end border radius of the component. | ||
* @prop --calcite-popover-text-color: The text color of the component. | ||
* @prop --calcite-popover-z-index: Sets the z-index value for the component. | ||
*/ | ||
|
||
:host { | ||
--calcite-floating-ui-z-index: var(--calcite-popover-z-index, theme("zIndex.popover")); |
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.
Related to above. This can be replaced with the following,
--calcite-floating-ui-z-index: var(--calcite-z-index-popup);
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 small thing for non-breaking changes on the z-index. Looks good 🎉
|
||
:host { | ||
--calcite-floating-ui-z-index: var(--calcite-popover-z-index, theme("zIndex.popover")); | ||
--calcite-floating-ui-z-index: var(--calcite-z-index-popup); |
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 non-breaking changes I just realized this should be
--calcite-floating-ui-z-index: var(--calcite-popover-z-index, var(--calcite-z-index-popup));
packages/calcite-components/src/components/popover/popover.scss
Outdated
Show resolved
Hide resolved
@Elijbet @alisonailea shouldn't these be converted to tokens? :host([scale="s"]) {
.heading {
@apply text-n1-wrap
px-3
py-2;
}
}
:host([scale="m"]) {
.heading {
@apply text-0-wrap
px-4
py-3;
}
}
:host([scale="l"]) {
.heading {
@apply text-1-wrap
px-5
py-4;
}
} |
No. We don't want to expose font size and padding as component tokens in most cases at the moment. |
What about border width?
or height?
|
Should we add a story for testing this? I notice the popover tail isn't being themed properly. |
Related Issue: #7180
Summary
Adds the following component tokens: