-
Notifications
You must be signed in to change notification settings - Fork 89
feat(card): component tokens #10161
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
Merged
Merged
feat(card): component tokens #10161
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
2623634
fix(card): simplify tokens
f91d2c3
fix(card, card-group): add component tokens to card-group and remove …
43fcc17
Merge branch 'dev' of github.com:Esri/calcite-design-system into ali-…
8b87d45
test(card): add storybook tests
32158f5
fix(card): update background color design pattern for hover and selected
2cbd1d8
test(card): add e2e token tests
daec090
fix(card): support checkbox-wrapper-deprecated
6e7b14c
test(card): clean up e2e for tokens
e3176db
fix(card): positioning of deprecated checkbox
ad4e375
Merge branch 'dev' of github.com:Esri/calcite-design-system into ali-…
33f0aa3
fix(card): update tokens
615dae6
test(card): fix component token e2e
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these still intended to be deprecated?
I would not expect to use “—calcite-card-background-color” to control the background color of the internal “checkbox element” like this deprecation message suggests.
The newly introduced names are confusing to me, there is no general hover or press state or interaction case for a Card component itself, those states should be applied separately via css prop imo. If a user wants to set calcite-card:hover { whatever } they can do so, but this internally rendered element is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not actually a checkbox component - it's just a styled div with an icon as it is in most other selection-enabled components.
The design I posted above with the multiple selection cards is a real-world use case I'd like to achieve (and is currently possible) - so removing these would prevent that design. We shouldn't be arbiters of theming choices - if we are truly making our components fully them able via tokens, everything should be theme able, not just what some deem appropriate.
I think this proposed nomenclature is more confusing - there is no hover state for the card - and we want folks to use stateful overrides (:hover, etc.) for actual parent-level adjustments based on those interactions right?
IMO setting
--calcite-card-background-color-hoverto control the bg of a portion of the card doesn't make sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not what we have discussed as a team. What we are providing is white labeling. We still have design patterns we have set and want to adhere to. That said, I can understand your argument against
--calcite-card-background-color-hover, I've updated the tokens. Is this an acceptable compromise?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good