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

Borders: Update border color for multiple components #2023

Merged
merged 2 commits into from Apr 4, 2022

Conversation

ayeshakmaz
Copy link
Contributor

@ayeshakmaz ayeshakmaz commented Mar 29, 2022

Summary

What changed?

Update Form Fields and Tables to use new border design Token. The main noticeable differences are in Switch, Checkbox, and RadioButton.

Switch

Before
Screen Shot 2022-03-29 at 1 51 36 PM

After
Screen Shot 2022-03-29 at 1 54 17 PM

Checkbox

Before
Screen Shot 2022-03-29 at 1 57 43 PM

After
Screen Shot 2022-03-29 at 1 57 55 PM

RadioButton

Before

Screen Shot 2022-03-29 at 1 58 11 PM

After
Screen Shot 2022-03-29 at 1 58 25 PM

Why?

In order to create consistent borders that are more accessible, this PR updates our form fields and tables to use our border design token colors. The main change in Controls components is designed to help out switches look more intentional, and to make disabled switches look more disabled. It also improves the accessibility because our previous color was not accessible against either our light ro dark backgrounds.

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)

@ayeshakmaz ayeshakmaz requested a review from a team as a code owner March 29, 2022 01:09
@@ -71,7 +71,7 @@ export default function Switch({
const sliderStyles = classnames(
styles.slider,
switched ? styles.sliderRight : styles.sliderLeft,
switched && !disabled ? styles.sliderDark : styles.sliderLight,
!disabled ? styles.sliderDark : styles.sliderLight,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intentional change to help our default switch look active, and our disabled switch to look more disabled

Copy link
Contributor

Choose a reason for hiding this comment

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

They still don't read as "disabled" enough to me, but this is an improvement! 😅 And that's beyond the scope of this PR

@ayeshakmaz ayeshakmaz added the patch release Patch release label Mar 29, 2022
@netlify
Copy link

netlify bot commented Mar 29, 2022

Deploy Preview for gestalt ready!

Name Link
🔨 Latest commit f4f791b
🔍 Latest deploy log https://app.netlify.com/sites/gestalt/deploys/624371ced1c27e00087e9d39
😎 Deploy Preview https://deploy-preview-2023--gestalt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@AlbertCarreras
Copy link
Contributor

AlbertCarreras commented Mar 29, 2022

@ayeshakmaz Intended?
After | Before
Screen Shot 2022-03-29 at 1 49 37 PM

@AlbertCarreras
Copy link
Contributor

These changes should have triggered the vis diff Integration test to fail... I wonder how it did pass it.

@ayeshakmaz
Copy link
Contributor Author

@AlbertCarreras made some updates and fixed that issue - thanks for taking a look!

@ponori
Copy link
Contributor

ponori commented Mar 30, 2022

Hey Ayesha - they may be a discrepancy between design and prod related to our disabled state. Just noting this as something we should align on in the future - it shouldn't block this PR.

@@ -71,7 +71,7 @@ export default function Switch({
const sliderStyles = classnames(
styles.slider,
switched ? styles.sliderRight : styles.sliderLeft,
switched && !disabled ? styles.sliderDark : styles.sliderLight,
!disabled ? styles.sliderDark : styles.sliderLight,
Copy link
Contributor

Choose a reason for hiding this comment

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

They still don't read as "disabled" enough to me, but this is an improvement! 😅 And that's beyond the scope of this PR

@ayeshakmaz ayeshakmaz merged commit a4d174c into pinterest:master Apr 4, 2022
@ayeshakmaz ayeshakmaz deleted the ayesha/border-roboflow branch April 4, 2022 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch release Patch release
Projects
None yet
4 participants