Skip to content
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(list, list-item, list-item-group): add component tokens #10669

Merged
merged 12 commits into from
Nov 20, 2024

Conversation

driskull
Copy link
Member

@driskull driskull commented Nov 1, 2024

Related Issue: #7180

Summary

adds new component tokens, e2e tests, demo page examples, and chromatic theme tests.

New CSS variables

List

--calcite-list-background-color: Specifies the component's background color.

ListItem

--calcite-list-background-color-hover: Specifies the component's background color when hovered.
--calcite-list-background-color-press: Specifies the component's background color when pressed.
--calcite-list-background-color: Specifies the component's background color.
--calcite-list-border-color: Specifies the component's border color.
--calcite-list-content-color: Specifies the content color.
--calcite-list-description-color: Specifies the description color.
--calcite-list-icon-color: Specifies the component's icon color.
--calcite-list-label-color: Specifies the label color.
--calcite-list-selection-border-color: Specifies the component's selection border color.

ListItemGroup

--calcite-list-background-color: Specifies the component's background color.
--calcite-list-color: Specifies the component's color.

# Conflicts:
#	packages/calcite-components/src/components/list-item/list-item.e2e.ts
#	packages/calcite-components/src/custom-theme.stories.ts
@driskull driskull changed the title Dris0000/list tokens 7180 feat(switch): add component tokens Nov 1, 2024
@driskull driskull changed the title feat(switch): add component tokens feat(list, list-item, list-item-group): add component tokens Nov 1, 2024
@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Nov 1, 2024
@driskull driskull requested a review from alisonailea November 1, 2024 21:37
Copy link
Contributor

@alisonailea alisonailea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these components need stateful tokens and because the components are all related, any shared values can have a shared token. For example --calcite-list-background-color can be used for list, list-item-group, and list-item

@driskull
Copy link
Member Author

driskull commented Nov 1, 2024

I don't think these components need stateful tokens

I think list-item does need stateful tokens. Hovering the whole host shouldn't change the background color. That wouldn't work for nested items.

@driskull driskull marked this pull request as ready for review November 4, 2024 17:31
*
* These properties can be overridden using the component's tag as selector.
*
* @prop --calcite-list-background-color: Specifies the component's background color.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d expect to support “—calcite-list-item-background-color” as well.

Consider this use case which is representative of how most of our users want to theme: https://community.esri.com/t5/calcite-design-system-questions/change-listitem-background-color-in-code/m-p/1557125#M767

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are limiting tokens when shared between grouped components. This is documented in the wiki.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different than Accordion Item / Accordion - where we have -item-background-color? #10181

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was wondering this as well. There are some inconsistencies here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alisonailea can you comment on the above? Currently, it seems like parent/child tokens are inconsistent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The “item tokens” path seems the easiest to follow both as a maintainer (IMO) and as someone who uses these theming options frequently day to day.

Using the Community post as just one example, it also seems to align with expectations from external folks - theming the element you want to adjust with component-specific nomenclature / rules.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need direction on this one. @alisonailea

* These properties can be overridden using the component's tag as selector.
*
* @prop --calcite-list-background-color-hover: Specifies the component's background color when hovered.
* @prop --calcite-list-background-color-press: Specifies the component's background color when pressed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above, does this need to be “list-item” - what hover would we actually be theming here on the parent?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are limiting tokens when shared between grouped components. This is documented in the wiki.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it is a bit weird to be targeting the parent hovering when its really the items being hovered.

# Conflicts:
#	packages/calcite-components/.stylelintrc.cjs
#	packages/calcite-components/src/components/list-item-group/list-item-group.e2e.ts
#	packages/calcite-components/src/components/list-item/list-item.e2e.ts
#	packages/calcite-components/src/components/list/list.e2e.ts
# Conflicts:
#	packages/calcite-components/src/custom-theme.stories.ts
@driskull
Copy link
Member Author

@alisonailea @macandcheese can you review again?

Copy link
Contributor

@alisonailea alisonailea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@driskull driskull added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Nov 20, 2024
@driskull driskull merged commit 12e3ff0 into dev Nov 20, 2024
15 checks passed
@driskull driskull deleted the dris0000/list-tokens-7180 branch November 20, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants