Feat/brand colors figma variables #698
Closed
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
garrettbear
added
documentation
|
@brianacnguyen @georgewrmarshall I noticed we need to add |
georgewrmarshall
requested changes
May 23, 2024
There was a problem hiding this comment.
Looking really good! Can we ensure that all Added, Changed and Removed CSS and JS tokens are listed in the migration docs. Also looks like there is an issue with displaying Figma tokens
Something like
Added
CSS
--brand-colors-grey-grey000
--brand-colors-grey-grey025
--brand-colors-grey-grey050
--brand-colors-grey-grey1000
--brand-colors-blue-blue025
--brand-colors-blue-blue050
// etc
JS
brandColor.grey000
brandColor.grey025
brandColor.grey050
//
Removed
CSS
--brand-colors-grey-grey030
--brand-colors-grey-grey040
--brand-colors-grey-grey750
--brand-colors-blue-blue000
--brand-colors-green-green000
--brand-colors-red-red000
// etc
JS
brandColor.grey030
brandColor.grey040
brandColor.grey750
brandColor.blue000
brandColor.green000
brandColor.red000
// etc
lightTheme.colors.overlay.inverse
lightTheme.colors.primary.shadow
lightTheme.colors.primary.disabled
lightTheme.colors.secondary
lightTheme.colors.error.shadow
// etc
Changed
CSS
--brand-colors-white-white000 to --brand-colors-white
--brand-colors-black-black000 to --brand-colors-black
JS
brandColor.white000 to brandColor.white
brandColor.black000 to `brandColor.black
docs/BrandColors.stories.tsx
Outdated
| @@ -51,7 +51,7 @@ export const CSS: Story = { | |||
| }, | |||
| }; | |||
|
|
|||
| export const JS: Story = { | |||
| export const JS = { | |||
There was a problem hiding this comment.
Is this change necessary? The other Stories are typed can we either remove it from all stories or add it here?
Suggested change
| export const JS = { | |
| export const JS: Story = { |
Comment on lines
-33
to
-37
| --color-secondary-default: var(--brand-colors-orange-orange400); | ||
| --color-secondary-alternative: var(--brand-colors-orange-orange300); | ||
| --color-secondary-muted: #f8883b26; | ||
| --color-secondary-inverse: var(--brand-colors-grey-grey900); | ||
| --color-secondary-disabled: #f8883b80; |
There was a problem hiding this comment.
Same for all these
Comment on lines
-34
to
-38
| --color-secondary-default: var(--brand-colors-orange-orange500); | ||
| --color-secondary-alternative: var(--brand-colors-orange-orange600); | ||
| --color-secondary-muted: #f66a0a19; | ||
| --color-secondary-inverse: var(--brand-colors-white-white000); | ||
| --color-secondary-disabled: #f66a0a80; |
There was a problem hiding this comment.
Add to migration docs
src/css/typography.css
Outdated
Comment on lines
4
to
6
|
|
||
| :root { | ||
| /* typography */ |
There was a problem hiding this comment.
nit: Can we revert these changes to keep code changes down?
Comment on lines
+7
to
+26
| Object.entries(brandColor).forEach(([key, value]) => { | ||
| const colorFamily = key.replace(/\d+.*$/u, ''); // Extracts 'grey' from 'grey000' | ||
| const shadeMatch = key.match(/\d+/gu); // Extracts ['000'] from 'grey000' | ||
| // if no numeric key is found, skip the test | ||
| if (!shadeMatch?.[0]) { | ||
| describe(`${colorFamily.toUpperCase()} `, () => { | ||
| it(`js tokens for ${key} matches figma brandColor ${colorFamily}`, () => { | ||
| const tokenValue = designTokens[colorFamily]?.value ?? ''; | ||
| expect(value.toLowerCase()).toStrictEqual(tokenValue.toLowerCase()); | ||
| }); | ||
| }); | ||
| return; | ||
| } | ||
| const shadeKey = shadeMatch[0]; // Ensures there's a valid shade key | ||
|
|
||
| describe(`${colorFamily.toUpperCase()} ${shadeKey}`, () => { | ||
| it(`js tokens for ${key} matches figma brandColor ${colorFamily}${shadeKey}`, () => { | ||
| const tokenValue = designTokens[colorFamily][shadeKey]?.value ?? ''; | ||
| expect(value.toLowerCase()).toStrictEqual(tokenValue.toLowerCase()); | ||
| }); |
13 tasks
MIGRATION.md
Outdated
Comment on lines
201
to
120
| - Grey updates | ||
| - grey000 added | ||
| - grey025 added | ||
| - grey050 added | ||
| - grey030 removed | ||
| - grey040 removed | ||
| - grey750 removed | ||
| - grey1000 added | ||
|
|
||
| - Blue updates | ||
| - blue000 removed | ||
| - blue025 added | ||
| - blue050 added | ||
| - blue400-15 added | ||
| - blue500-10 added | ||
|
|
||
| - Green updates | ||
| - green000 removed | ||
| - green025 added | ||
| - green050 added | ||
| - green400-15 added | ||
| - green500-10 added | ||
|
|
||
| - Red updates | ||
| - red000 removed | ||
| - red025 added | ||
| - red050 added | ||
| - red400-15 added | ||
| - red500-10 added | ||
|
|
||
| - Yellow updates | ||
| - yellow000 removed | ||
| - yellow025 added | ||
| - yellow050 added | ||
| - yellow100-15 added | ||
| - yellow400-15 added | ||
| - yellow500-10 added | ||
| - yellow700 added | ||
| - yellow800 added | ||
| - yellow900 added | ||
|
|
||
| - Orange updates | ||
| - orange000 removed | ||
| - orange025 added | ||
| - orange050 added | ||
|
|
||
| - Violet removed | ||
|
|
||
| - Purple added | ||
|
|
||
| - Lime added | ||
|
|
||
| - White and black updates | ||
| - white000 renamed to white | ||
| - white010 removed | ||
| - black000 renamed to black | ||
|
|
||
| ### Brand Colors Brand Evolution | ||
|
|
||
| - Blue updates | ||
| - blue300-15 added | ||
| - blue400-15 removed | ||
|
|
||
| - Green updates | ||
| - green300-15 added | ||
| - green400-15 removed | ||
|
|
||
| - Red updates | ||
| - red300-15 added | ||
| - red400-15 removed | ||
|
|
||
| ### Themed Colors | ||
|
|
||
| - overlay-inverse removed | ||
| - primary-shadow removed | ||
| - primary-disabled removed | ||
| - secondary colors removed | ||
| - error-shadow removed | ||
| - error-disabled removed | ||
| - warning-alternative removed | ||
| - warning-disabled removed | ||
| - success-alternative removed | ||
| - success-disabled removed | ||
| - info-alternative removed | ||
| - info-disabled removed | ||
| - network colors removed | ||
| - shadow-primary added | ||
| - shadow-error added | ||
| - button colors removed |
There was a problem hiding this comment.
These can be removed once they have be cross referenced with the JS and CSS above
There was a problem hiding this comment.
@georgewrmarshall you'll need to be a little more specific. I'm not completely understand how you want this written out
There was a problem hiding this comment.
I've listed the specific JS and CSS tokens that engineers should check for. For example, what does "button colors" mean? What do "network colors" refer to? Without these specifics, it's difficult for engineers to understand what to look for when identifying removed design token code. This list should also be used to check for any removed CSS variables.
garrettbear
force-pushed
the
feat/brand-colors-figma-variables
branch
from
3af0066
to
102e30c
Compare
13 tasks
garrettbear
commented
May 23, 2024
garrettbear
commented
May 24, 2024
docs/utils/useJsonColor.ts
Outdated
There was a problem hiding this comment.
This file does still currently have some lint errors/warnings but is operational
13 tasks
CHANGELOG.md
Outdated
| @@ -260,7 +290,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| - Initial release. | |||
|
|
|||
| [Unreleased]: https://github.com/MetaMask/design-tokens/compare/v2.1.1...HEAD | |||
| [Unreleased]: https://github.com/MetaMask/design-tokens/compare/v3.0.0...HEAD | |||
| [3.0.0]: https://github.com/MetaMask/design-tokens/compare/v2.1.1...v3.0.0 | |||
| [2.1.1]: https://github.com/MetaMask/design-tokens/compare/v2.1.0...v2.1.1 | |||
| [2.1.0]: https://github.com/MetaMask/design-tokens/compare/v2.0.3...v2.1.0 | |||
| [2.0.3]: https://github.com/MetaMask/design-tokens/compare/v2.0.2...v2.0.3 | |||
There was a problem hiding this comment.
Revert these changes
vite.config.mjs
Outdated
| }, | ||
| server: { | ||
| open: '/index.html', | ||
| }, |
There was a problem hiding this comment.
Remove this file
georgewrmarshall
force-pushed
the
feat/brand-colors-figma-variables
branch
from
e89d993
to
f3784f7
Compare
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.
Description
Update to start using figma color variables for
brandColor,lightTheme, anddarkTheme. This involves a new json architect from Figma.Deprecatedhave officially been removedRelated issues
Fixes: #660 #660
Manual testing steps
Screenshots/Recordings
Before
before720.mov
After
Screen.Recording.2024-05-22.at.4.35.03.PM.mov
Pre-merge author checklist
Pre-merge reviewer checklist