Support primary and accent variants for toggle and menu buttons #1934
Conversation
This file contains 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
m-akinc
commented
Mar 15, 2024
m-akinc
commented
Mar 15, 2024
m-akinc
commented
Mar 15, 2024
|
@msmithNI do you mind buddying this? |
msmithNI
approved these changes
Mar 18, 2024
change/@ni-nimble-components-083cf44c-68b4-43f6-b689-efd81c8228b4.json
Outdated
Show resolved
Hide resolved
rajsite
reviewed
Mar 18, 2024
1 task
jattasNI
approved these changes
Mar 20, 2024
m-akinc
added a commit
that referenced
this pull request
Mar 21, 2024
## 🤨 Rationale
I encountered a couple design tokens with names that felt inconsistent:
`buttonFillActivePrimaryColor`
`buttonFillAccentActiveColor`
It seemed like the appearance variant ("accent" or "primary") should be
in the same relative position within the name, but our naming convention
dictated these names instead.
## 👩💻 Implementation
Treat common state descriptors ("active", "disabled", "hover", and
"selected") as a separate category of name segment than other
distinguishing descriptors (like "accent" and "primary"). So instead of
our old structure:
`[element]-[part]-[state]-[token_type]`
we now have:
`[element]-[part]-[state]-[variant]-[token_type]`
Luckily, the only existing token that violates this updated naming
scheme is `buttonFillAccentActiveColor` (which should instead be
`buttonFillActiveAccentColor`) and that token is being removed in
[another PR](#1934).
## 🧪 Testing
N/A
## ✅ Checklist
- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
---------
Co-authored-by: Milan Raj <[email protected]>
rajsite
reviewed
Mar 22, 2024
Comment on lines
14
to
22
| export const appearanceStates: [string, string | undefined][] = Object.entries( | ||
| ButtonAppearance | ||
| ).map(([key, value]) => [pascalCase(key), value]); | ||
| export type AppearanceState = (typeof appearanceStates)[number]; | ||
|
|
||
| export const appearanceVariantStates: [string, string | undefined][] = Object.entries(ButtonAppearanceVariant).map(([key, value]) => [ | ||
| pascalCase(key), | ||
| value | ||
| ]); |
There was a problem hiding this comment.
You lose as const-ness typing of the enum states this way. Also it's not easier to read and not really changing often so don't think being explicit is a maintenance burden. Think it's better to have the stronger type
Compared to
There was a problem hiding this comment.
I had just copied these as they were. I'll change them.
rajsite
reviewed
Mar 22, 2024
rajsite
approved these changes
Mar 26, 2024
fredvisser
approved these changes
Mar 26, 2024
TrevorKarjanis
approved these changes
Mar 26, 2024
1 task
rajsite
added a commit
that referenced
this pull request
Mar 28, 2024
# Pull Request ## 🤨 Rationale Makes the following bits more consistent in the storybook matrix tests: - All the states enums use the normal enum pattern ([instead of programmatic enum definition which looses some typing](#1934 (comment))) - All the states are defined first before the storybook metadata definition - All the states use the naming convention of `blahStates` for the state dimension and `BlahState` for the individual state type - File specific states were put in alphabetical order ## 👩💻 Implementation See above. ## 🧪 Testing Should not result in any changes in stories except for one change in dialogs where I have the state name render on the page instead of the state value which is a long CSS string. ## ✅ Checklist - [x] I have updated the project documentation to reflect my changes or determined no changes are needed. Hoping folks just copy and paste the patterns, didn't seem worth a doc update.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Pull Request
🤨 Rationale
Fixes: #1906
👩💻 Implementation
appearance-variantattribute to both toggle and menu button.patterns/buttons/tests.DigitalGreenDarktoken value was incorrect, based on the Figma document. I updated this value, which affected block-accent button color and the rich-text mention font color.buttonFillActivePrimaryColorandbuttonFillAccentActiveColor.To avoid increasing the size of this PR, Angular/Blazor support will be added in a follow-up PR.
🧪 Testing
Lots of manual state styling testing in Storybook. Also Chromatic visual tests.
✅ Checklist