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

Box: Add new background colors #1867

Merged
merged 11 commits into from
Feb 1, 2022

Conversation

ayeshakmaz
Copy link
Contributor

@ayeshakmaz ayeshakmaz commented Jan 18, 2022

Summary

What changed?

Update Box to support new design token based colors.

Why?

We're working on transitioning away from color names for Box backgrounds ("eggplant") and over to purpose-based design tokens (like "infoBase"). This PR adds the new options as possibilities and updates the documentation. A future PR + codemod will properly deprecate all the old values.

Links

Checklist

  • Added unit and Flow Tests
  • Added documentation + accessibility tests
  • Verified accessibility: keyboard & screen reader interaction
  • Checked dark mode, responsiveness, and right-to-left support
  • Checked stakeholder feedback (e.g. Gestalt designers)

@netlify
Copy link

netlify bot commented Jan 18, 2022

✔️ Deploy Preview for gestalt ready!

🔨 Explore the source changes: a49e586

🔍 Inspect the deploy log: https://app.netlify.com/sites/gestalt/deploys/61f85614a7982f00079cc68e

😎 Browse the preview: https://deploy-preview-1867--gestalt.netlify.app

@ayeshakmaz ayeshakmaz marked this pull request as ready for review January 18, 2022 19:27
@ayeshakmaz ayeshakmaz requested a review from a team as a code owner January 18, 2022 19:27
@ayeshakmaz ayeshakmaz added the minor release Minor release label Jan 18, 2022
description={`
The following colors can be used to change the background of Box.

⚠️ Note that the previous options ('red', 'white', 'lightGray', 'gray', 'darkGray', 'green', 'pine', 'olive', 'blue', 'navy', 'midnight', 'purple', 'orchid', 'eggplant', 'maroon', 'watermelon', 'orange') are still valid but will be deprecated soon.
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding a section helping with this transition? A table or something that shows "prevColor --> newColor"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have this mapping yet (part of why I've split this into 2 PRs), but I'll definitely do that for the next PR to help show which are deprecated vs which have new color mappings

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if it's easy, can we display "inverse" on a dark background?

Copy link
Contributor

@dangerismycat dangerismycat left a comment

Choose a reason for hiding this comment

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

This looks fine — I imagine you're coordinating merging with a wider announcement about the change? How do you plan to address the temporary mismatch between the colors used in code and the ones we show examples of in the docs?

description={`
The following colors can be used to change the background of Box.

⚠️ Note that the previous options ('red', 'white', 'lightGray', 'gray', 'darkGray', 'green', 'pine', 'olive', 'blue', 'navy', 'midnight', 'purple', 'orchid', 'eggplant', 'maroon', 'watermelon', 'orange') are still valid but will be deprecated soon.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if it's easy, can we display "inverse" on a dark background?

@ayeshakmaz
Copy link
Contributor Author

Definitely going to pair with an announcement. For the docs, I'm going to leave as is, and once we deprecate the other colors, I'll build an internal component for the Box doc examples

@AlbertCarreras
Copy link
Contributor

@ayeshakmaz Some of the new colors infoBase, shopping, education are the same color. Should we add any guidance on which color names to use when same color apply. I can see cases where infoBase, shopping, education are used interchangeably and future under the hood changes in the color values producing errors.

@ayeshakmaz
Copy link
Contributor Author

I can add a line about using these colors based on their intended purposes, given by the name. So only use education if you are showing an educational UI element for example

@ayeshakmaz ayeshakmaz merged commit 1e5f252 into pinterest:master Feb 1, 2022
@ayeshakmaz ayeshakmaz deleted the ayesha/box-design-tokens branch February 1, 2022 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor release Minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants