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

Initialize Tokens 2.0 #1790

Merged
merged 10 commits into from
May 31, 2023
Merged

Initialize Tokens 2.0 #1790

merged 10 commits into from
May 31, 2023

Conversation

edburyenegren-okta
Copy link
Contributor

@edburyenegren-okta edburyenegren-okta commented May 10, 2023

Description

This is a WIP PR for initializing the new odyssey-design-tokens schema.

Current changes

  • Moves border colors under border; removes color/border
  • Moves text colors under font; removes color/text
  • Removes unused color/background tokens
  • Renames color/palette to hue
  • Renames color/functional to palette (matching MUI use)
    • Restructures palette schema to match MUI
  • Renames space to spacing; moves 0 key to 0 value (matching MUI use)
  • Refactors MUI and Storybook to use new token schema

ToDo

@@ -1,14 +1,28 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file consolidates all border tokens.

@@ -0,0 +1,32 @@
{
"palette": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently, these map to what's available in MUI. however, the palette proposed by Vis has many more tokens, and their use cases are clearer.

@taylorridgway-okta is currently reviewing our actual use cases for various hues to make better abstractions.

For now, I am partial to leaving these as-is and refactoring them as we implement the rebranded UI. that might make discoverability simpler.

Copy link

@taylorridgway-okta taylorridgway-okta left a comment

Choose a reason for hiding this comment

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

Overall, I really appreciate the cleaner token naming. Helps a lot with readability

packages/odyssey-design-tokens/src/border.json Outdated Show resolved Hide resolved
"comment": "Branded text color, primarily used within button",
"value": "{palette.primary.main.value}"
},
"sub": {

Choose a reason for hiding this comment

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

Naming here is really tricky, we currently name them Primary and Secondary. I've floated Prominent and Recessive in the past, but realize this is a description of the hierarchy not their use.

Subordinate makes sense once I think about it, but I'm not sure it's super intuitive.

Want to make sure we're intentional about how the recessive gray text is named. What do you think @gretchen-okta ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

We use subtitle and caption in other places. It'd be nice to get consistent there.

MUI uses subtitle, but we can override that / add a new type.

Copy link

@gretchen-okta gretchen-okta May 30, 2023

Choose a reason for hiding this comment

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

To look at it as a system... is the recessive gray text 600 and we also need a 700? Is the recessive use (600?) secondarily used vs 900 as primary?

When 700 is added so we have a scale of 3....
Main (900)
Light (700)
Lightest (600)
OR
Primary (900)
Secondary (now 700)
Tertiary (600; this use is unique in our taxonomy so far)

Prominent could be interpreted as more important than primary/main...like headline/subtitle?

The other use of primary and secondary color is for icon tokens. If we change text, do icon need to shift so we describe values and use in a similar way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not too attached to having second-order variants share similar names unless it's straightforward. I would like to stay away from hierarchies when it comes to functional tokens because they're arbitrary.

body, heading, and bodyInverse seem clear and likely won't show up anywhere outside of typography.

danger makes to me, since the use case is clear.

I think primary was named as such simply because it matches our palette.primary, but it doesn't really spell out the case. action might be more intuitive.

Re: sub, I think I'm most partial to either subtitle (potentially confusing but matches MUI) and caption (not a 1:1 use case, but doesn't rely on hierarchy).

This contrasts with Button, where the primary variant indicates an in-UI hierarchy of importance.

Choose a reason for hiding this comment

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

@edburyenegren-okta I don't know if I can get typography scale/use (like subtitle, body small) aligned with color tokens... Should I try to align color uses with color? Suggestions above either align with icon color tokens (primary, secondary) or hue/primary, error, warning, success (main, light, lightest)

A third idea:
900 could easily be body
but
700 = tag labels, table column headers, and areas of system design ...label feels closer than subheading or header to describe use of these cases
600 = secondary, supporting content in the UI to set it back a level hierarchically ....body light (related to body small?)

Choose a reason for hiding this comment

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

I'm not too attached to having second-order variants share similar names unless it's straightforward. I would like to stay away from hierarchies when it comes to functional tokens because they're arbitrary.

body, heading, and bodyInverse seem clear and likely won't show up anywhere outside of typography.

danger makes to me, since the use case is clear.

I think primary was named as such simply because it matches our palette.primary, but it doesn't really spell out the case. action might be more intuitive.

Re: sub, I think I'm most partial to either subtitle (potentially confusing but matches MUI) and caption (not a 1:1 use case, but doesn't rely on hierarchy).

This contrasts with Button, where the primary variant indicates an in-UI hierarchy of importance.

I think I can get behind use of color but can we tweak a few:
body
bodyInverse
error (more consistent vs danger)
link (if that's what we mean by action for typography color)

  • if we are naming by use, then I can't tell where subtitle or caption are used because we really don't have them? Would 'label' work (for 700 use case of tag label and table column header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error works for me! Button might be the only case we use "danger" because it's a user action rather than user-facing info.

link might be misleading because we also use it for Tabs and some Buttons.

caption/sub/secondary - I just did a quick inventory. We use this for:

  • Field hints
  • legalese
  • actual captions (e.g. the figcaption under an image or graph)
  • keyboard keys / kbd
  • Chips (disabled text & icons)
  • IconButton (disabled)
  • Link (monochrome hover)
  • List Subheaders (e.g. option groups in Select or Menu)
  • Table footers, headers, and sort indicators

This isn't representative of the latest Rebrand UI color usage; however, the fact that they may change is probably a good indicator that they may need different tokens even if they coincidentally share the same value.

"value": "{hue.neutral.600.value}"
},
"danger": { "value": "{palette.danger.main.value}" }
},

Choose a reason for hiding this comment

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

Also flagging: it's incredibly likely that we add another token for text at 700. Need to find a way to describe use case(s) to help with tokenization

Choose a reason for hiding this comment

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

Ok, will revert to danger and action to cover more use cases:
body
bodyInverse
danger
action

Can caption/sub/secondary be 'secondary'? There are so many varied uses, it needs to be broad...

"dark": { "value": "{hue.neutral.900.value}" }
}
}
}

Choose a reason for hiding this comment

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

Pretty sure this looks good, just want to make absolutely sure that main will always be 3.0:1 and below contrast ratio a11y requirement and won't be used, for example, for text anywhere.

packages/odyssey-react-mui/src/theme/palette.ts Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/theme/palette.ts Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/theme/palette.ts Outdated Show resolved Hide resolved
packages/odyssey-design-tokens/src/palette.json Outdated Show resolved Hide resolved
packages/odyssey-design-tokens/src/palette.json Outdated Show resolved Hide resolved
packages/odyssey-design-tokens/src/palette.json Outdated Show resolved Hide resolved
@edburyenegren-okta edburyenegren-okta changed the title [WIP] Initialize Tokens 2.0 Initialize Tokens 2.0 May 17, 2023
Copy link
Contributor

@ganeshsomasundaram-okta ganeshsomasundaram-okta left a comment

Choose a reason for hiding this comment

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

looks good to me!

ganeshsomasundaram-okta

This comment was marked as duplicate.

Copy link
Contributor

@jordankoschei-okta jordankoschei-okta 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, no suggestions!

@edburyenegren-okta edburyenegren-okta merged commit 6eafaf9 into develop May 31, 2023
@edburyenegren-okta edburyenegren-okta deleted the ee/tokens-2-color branch May 31, 2023 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants