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(dropdown, dropdown-item, dropdown-group): add component tokens with fallbacks #8870

Merged
merged 13 commits into from
Mar 19, 2024

Conversation

alisonailea
Copy link
Contributor

Related Issue: #7180

Summary

Dropdown

--calcite-dropdown-background-color: specifies the background color of the component.

Dropdown Item

--calcite-dropdown-item-background-color: specifies the background color of the component.
--calcite-dropdown-item-icon-color: specifies the color of an icon in the component.
--calcite-dropdown-item-text-color: specifies the text color of the component.

Dropdown Group

--calcite-dropdown-group-border-color: specifies the border color of the component.
--calcite-dropdown-group-text-color: specifies the text color of the component.

Split Button

--calcite-split-button-dropdown-background-color: defines the background color of the component's dropdown.

@alisonailea alisonailea requested a review from a team as a code owner March 2, 2024 04:39
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Looking good! Just had a question regarding stateful CSS props.

@alisonailea alisonailea added enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Mar 14, 2024
@alisonailea alisonailea requested a review from jcfranco March 14, 2024 20:57
@alisonailea alisonailea added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Mar 14, 2024
@alisonailea alisonailea added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Mar 15, 2024
@@ -1,12 +1,16 @@
export const CSS = {
actionIndicator: "action-indicator",
Copy link
Member

Choose a reason for hiding this comment

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

Are these changes relevant to dropdown? I don't think dropdown components use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was an old class that already existed here but it wasn't in use. I added it back to remove the need for an unnecessary mixin in the styles.

iconContainer: "icon-container",
indicatorText: "indicator-text",
indicatorWithIcon: "indicator-with-icon",
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: -- should be used as a delimiter to state/modifiers. Also, one could be dropped if either with or without icon is treated as the default (e.g., indicator + indicator--icon).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the old classes. Would you like me to add these as a refactor within this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. We can do this separately then. Might be good to do a quick audit just to see how we're doing there. Very low priority though.

// Selection Indicator Icon
calcite-icon.dropdown-item-icon {
--calcite-icon-color: var(
--calcite-dropdown-item-indicator-color,
Copy link
Member

Choose a reason for hiding this comment

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

Sidebar: I think I've been implementing this pattern a bit differently (the public prop is resolved first when assigning internal CSS props). Will review and update accordingly.

@alisonailea alisonailea requested a review from jcfranco March 19, 2024 16:37
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Awesome! Good to merge once action changes are rolled back. 🚀

@alisonailea alisonailea merged commit 3049ac4 into epic/7180-component-tokens Mar 19, 2024
4 checks passed
@alisonailea alisonailea deleted the astump/7180-dropdown branch March 19, 2024 21:45
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.

2 participants