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(navigation-logo): adds component tokens #8894

Conversation

anveshmekala
Copy link
Contributor

@anveshmekala anveshmekala commented Mar 6, 2024

Related Issue: #7180

Summary

Adds the following tokens in calcite-navigation-logo component

--calcite-navigation-logo-background-color: Specifies the background color of the component.
--calcite-navigation-logo-border-color: Specifies the border color of the component.
--calcite-navigation-logo-description-text-color: Specifies text color of description of component.
--calcite-navigation-logo-heading-text-color: Specifies the text color of heading of component.
--calcite-navigation-logo-text-color: Specifies the text color of the component.

@anveshmekala anveshmekala added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Mar 6, 2024
@anveshmekala anveshmekala 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 6, 2024
@anveshmekala anveshmekala marked this pull request as ready for review March 6, 2024 23:32
@anveshmekala anveshmekala requested a review from a team as a code owner March 6, 2024 23:32
@anveshmekala anveshmekala 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 7, 2024
@anveshmekala anveshmekala 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 7, 2024
@anveshmekala anveshmekala 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 8, 2024
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.

Per Adam's comment. I don't think we need stateful tokens here. I've shown three ways to get to the tokens we need here. I'd vote 1. or 2. but it's up to you.

@anveshmekala
Copy link
Contributor Author

I don't think we need stateful tokens here.

This part i am bit confused. Initially had them and later removed them and added back. The refactor doc says tokens should be added for appropriate states. I can see the reasoning for adding them since hover and active are appropriate states for navigation-logo component. On the other hand, user can simply set the state on component selector and change the token variable if required.

Are we adding state tokens for components which are inaccessible to the user (inside shadowDOM) or components which can used as sub-components for other components?

@anveshmekala anveshmekala 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 8, 2024
@anveshmekala anveshmekala requested a review from alisonailea March 8, 2024 21:36
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.

🎉 Looks great! Thanks

@anveshmekala anveshmekala merged commit 37eaa27 into epic/7180-component-tokens Mar 8, 2024
8 checks passed
@anveshmekala anveshmekala deleted the anveshmekala/7180-navigation-logo-tokens branch March 8, 2024 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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