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(tab-title): add icon start/end custom CSS prop #10871

Merged

Conversation

jcfranco
Copy link
Member

@jcfranco jcfranco commented Nov 25, 2024

Related Issue: #10497

Summary

Adds the following component tokens (CSS props):

--calcite-tab-icon-color-end – Specifies the component's `iconEnd` color. Fallback to `--calcite-icon-color`.
--calcite-tab-icon-color-start – Specifies the component's `iconStart` color. Fallback to `--calcite-icon-color`.

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Nov 25, 2024
@jcfranco jcfranco force-pushed the jcfranco/10497-add-tab-title-icon-start-end-color-css-props branch from 83ef283 to 4832009 Compare November 25, 2024 21:47
@jcfranco
Copy link
Member Author

@alisonailea @driskull @macandcheese I couldn’t find an updated guideline in the wiki, but this ties back to our conversation last week about parent-child prop/token naming. I might need to rename the prop to use tabs instead of tab. Can you confirm? I can make the necessary tweaks.

*
* These properties can be overridden using the component's tag as selector.
*
* @prop --calcite-tab-end-icon-color: Specifies the component's `iconEnd` color. Fallback to `--calcite-icon-color`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this make more sense as --calcite-tab-icon-start-color and --calcite-tab-icon-end-color instead of start/end-icon?

That would match the property names of iconStart and iconEnd (not startIcon / endIcon)?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I had originally, but changed to follow accordion-item.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could confirm that - that does seem like it should match the property name? If it is intended to be the opposite of prop name, feel free to ignore.

Copy link
Contributor

@alisonailea alisonailea Nov 26, 2024

Choose a reason for hiding this comment

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

Per the naming schema https://github.com/Esri/calcite-design-system/wiki/tokens-naming-schema is should be [system]-[component]-[element]-[type]-[appearance] which would be --calcite-tab-icon-color-start/end

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on ☝️, we'll need to correct accordion-item's props too. cc @geospatialem

@alisonailea Can you update the schema diagram to include start/end as examples?

Copy link
Member

Choose a reason for hiding this comment

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

Added #10894 to next month's December milestone for the accordion-item token refactor. cc @alisonailea

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated the schema diagram to include start/end as examples

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 @macandcheese Any additional feedback on the name and PR besides start/end?

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM !

@jcfranco jcfranco added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Nov 27, 2024
@jcfranco jcfranco 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 Nov 27, 2024
@jcfranco jcfranco force-pushed the jcfranco/10497-add-tab-title-icon-start-end-color-css-props branch from 0e0ee0a to 90b7213 Compare November 27, 2024 03:09
@jcfranco jcfranco 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 Nov 27, 2024
@jcfranco
Copy link
Member Author

Merging per #10871 (comment). @alisonailea LMK there's anything else that needs to be addressed.

@jcfranco jcfranco merged commit c988ad1 into dev Nov 27, 2024
13 checks passed
@jcfranco jcfranco deleted the jcfranco/10497-add-tab-title-icon-start-end-color-css-props branch November 27, 2024 03:32
@github-actions github-actions bot added this to the 2024-12-17 - Dec Milestone milestone Nov 27, 2024
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.

4 participants