-
Notifications
You must be signed in to change notification settings - Fork 78
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(tabs, tab-nav, tab-title, tab): add component tokens #11720
base: dev
Are you sure you want to change the base?
Conversation
…ciado88/7180-add-tabs-design-tokens
…ciado88/7180-add-tabs-design-tokens
…ciado88/7180-add-tabs-design-tokens
…ciado88/7180-add-tabs-design-tokens
…ciado88/7180-add-tabs-design-tokens
@@ -25,10 +25,11 @@ import { TabID, TabLayout, TabPosition } from "../tabs/interfaces"; | |||
import { getIconScale } from "../../utils/component"; | |||
import { IconNameOrString } from "../icon/interfaces"; | |||
import { isBrowser } from "../../utils/browser"; | |||
import { XButton } from "../functional/XButton"; |
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.
Do we want to separate this x button refactor into a separate PR (applies to changes to other components like Button, Combobox)
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.
@jcfranco Should we split this up?
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 it's fine, but I'll defer to you. IIRC, the ✖️ button needed to be adjusted for the tabs tokens work.
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 can install this as is 🚀
|
||
padding-block: var( | ||
--calcite-tab-content-space-y, | ||
var(--calcite-tab-content-block-padding, var(--calcite-internal-tabs-content-space-y-fallback)) |
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.
Is this -fallback
portion needed?
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 can take the -fallback
bit out
@@ -195,7 +195,7 @@ export class Tabs extends LitElement { | |||
return ( | |||
<> | |||
<slot name={SLOTS.titleGroup} /> | |||
<section> | |||
<section class={CSS.section}> |
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.
Don't think this is required. We can leverage tag selector in theme tests.
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.
Applies to resource & e2e files.
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.
@anveshmekala I tried using tag selection but the test won't pass. I looked at other themed tests and couldn't find a tag selection example setup that works, other than using the section
class as a selector.
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.
Interesting, it is working for me as expected. I might be missing something here. Attached ref #11778
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.
It works using your setup, I'll update the PR with your update. Thank you!
…ciado88/7180-add-tabs-design-tokens
} | ||
|
||
&:host(:hover) .container::after { | ||
background-color: var(--calcite-tab-background-color, var(--calcite-color-foreground-2)); |
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 this should be --calcite-tab-background-color-hover
to customize the hover background-color which has a different default value.
Applies to other places in this file.
@@ -4,12 +4,13 @@ export const ICON = { | |||
} as const; | |||
|
|||
export const CSS = { | |||
container: "tab-nav", | |||
container: "container", |
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.
Nitpick: No longer required.
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.
Great stuff! Have a few doc-consistency suggestions for consideration. 📚
packages/calcite-components/src/components/tab-nav/tab-nav.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/tab-nav/tab-nav.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/tab-title/tab-title.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/tab-title/tab-title.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/tab-title/tab-title.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/tab-title/tab-title.scss
Outdated
Show resolved
Hide resolved
**Related Issue:** # ## Summary
…ciado88/7180-add-tabs-design-tokens
…ciado88/7180-add-tabs-design-tokens
…ciado88/7180-add-tabs-design-tokens
…ciado88/7180-add-tabs-design-tokens
…ciado88/7180-add-tabs-design-tokens
…ciado88/7180-add-tabs-design-tokens
…ciado88/7180-add-tabs-design-tokens
…ciado88/7180-add-tabs-design-tokens
@ashetland @SkyeSeitz Can you please take a look? |
Related Issue: #7180
Summary
Add css tokens to the following components.
Tabs
--calcite-tab-background-color
: Whenbordered
, specifies the component's background color.--calcite-tab-border-color
: Specifies the component's border color.Tab-nav
--calcite-tab-background-color
: Whencalcite-tabs
isbordered
, specifies the component's background color.--calcite-tab-border-color
: Whencalcite-tabs
isbordered
, specifies the component's border color.--calcite-tab-text-color
: Specifies the component'siconStart
,iconEnd
, and text color.Tab-title
--calcite-tab-text-color
: Specifies the component's text color.--calcite-tab-border-color
: Specifies the component's border color.--calcite-tab-background-color
: Specifies the component's background color.--calcite-tab-background-color-hover
: Whencalcite-tabs
isbordered
, specifies the component's background color when hovered.--calcite-tab-accent-color
: Specifies the component's accent color.--calcite-tab-accent-color-hover
: Specifies the component's accent color when hovered.--calcite-tab-accent-color-press
: Specifies the component's accent color when selected or active.--calcite-tab-close-icon-color
: Specifies the component's close element icon color.--calcite-tab-close-icon-color-press
: Specifies the component's close element icon color when hovered, focused, and active.--calcite-tab-close-icon-background-color
: Specifies the component's close element icon background color.--calcite-tab-close-icon-background-color-press
: Specifies the component's close element icon background color when hovered, focused, and active.Tab
--calcite-tab-content-space-y
: Specifies the vertical space between the component's content in thedefault
slot.