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(select): add component tokens #11178

Merged
merged 16 commits into from
Jan 10, 2025

Conversation

aPreciado88
Copy link
Contributor

@aPreciado88 aPreciado88 commented Dec 31, 2024

Related Issue: #7180

Summary

Add select component tokens.

--calcite-select-font-size: Specifies the font size of calcite-options in the component.
--calcite-select-text-weight: Specifies the font weight of calcite-options in the component.
--calcite-select-text-color: Specifies the text color of calcite-options in the component.
--calcite-select-border-color: Specifies the component's border color.
--calcite-select-icon-color: Specifies the component's icon color.
--calcite-select-icon-color-hover: Specifies the component's icon color when hovered or active.

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Dec 31, 2024
@aPreciado88 aPreciado88 marked this pull request as ready for review January 6, 2025 18:39
@aPreciado88
Copy link
Contributor Author

@alisonailea I'm having some issues testing some of the tokens, they're commented out in select.e2e. Could you please take a look?

* @prop --calcite-select-text-weight: Specifies the font weight of `calcite-option`s in the component.
* @prop --calcite-select-spacing-inline: Specifies the inline padding of `calcite-option`s in the component.
* @prop --calcite-select-text-color: Specifies the text color of `calcite-option`s in the component.
* @prop --calcite-select-size: Specifies the component's width.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this one, as the user can specify this on the element itself.

* @prop --calcite-select-spacing-inline: Specifies the inline padding of `calcite-option`s in the component.
* @prop --calcite-select-text-color: Specifies the text color of `calcite-option`s in the component.
* @prop --calcite-select-size: Specifies the component's width.
* @prop --calcite-select-block-size: Specifies the component's height.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this one, as the user can specify this on the element itself.

* @prop --calcite-select-text-color: The text color of the component.
* @prop --calcite-select-font-size: Specifies the font size of `calcite-option`s in the component.
* @prop --calcite-select-text-weight: Specifies the font weight of `calcite-option`s in the component.
* @prop --calcite-select-spacing-inline: Specifies the inline padding of `calcite-option`s in the component.
Copy link
Contributor

Choose a reason for hiding this comment

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

This one shouldn't be exposed.

@aPreciado88 aPreciado88 added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jan 8, 2025
@aPreciado88
Copy link
Contributor Author

@ashetland @SkyeSeitz Can you please take a look at the new screenshots?

Copy link
Contributor

@macandcheese macandcheese left a comment

Choose a reason for hiding this comment

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

Couple q -

@ashetland @SkyeSeitz - do we need to expose these text-weight / font-size properties here?

@alisonailea - there is just one focusable / hover-able container here - does this need the "--select-icon-color-hover" - or should a user use calcite-select:hover { etc... }

@ashetland
Copy link
Contributor

ashetland commented Jan 9, 2025

do we need to expose these text-weight / font-size properties here?

I know we needed to make a font weight change on the Select used in Date Picker, but I think these could stay internal for now

@aPreciado88 aPreciado88 removed the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jan 9, 2025
@aPreciado88 aPreciado88 added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jan 9, 2025
@aPreciado88
Copy link
Contributor Author

@ashetland Can you please take a look at the latest snapshots?

@aPreciado88 aPreciado88 removed the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jan 9, 2025
@aPreciado88 aPreciado88 added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jan 10, 2025
@aPreciado88 aPreciado88 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 Jan 10, 2025
@aPreciado88 aPreciado88 added pr ready for visual snapshots Adding this label will run visual snapshot testing. skip visual snapshots Pull requests that do not need visual regression testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jan 10, 2025
@aPreciado88 aPreciado88 merged commit e878e07 into dev Jan 10, 2025
10 checks passed
@aPreciado88 aPreciado88 deleted the aPreciado88/7180-add-select-design-tokens branch January 10, 2025 21:00
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. skip visual snapshots Pull requests that do not need visual regression testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants