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.

Sign up for GitHub

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

Jump to bottom

[Button] New design language #2746

Merged
merged 2 commits into from
Feb 21, 2020
Merged

[Button] New design language #2746

merged 2 commits into from
Feb 21, 2020

Conversation

kyledurand
Copy link
Member

This PR is meant to be the final push from partially completed in React to full

https://github.com/Shopify/polaris-ux/issues/371

I'd add some screenshots but I feel that would be ineffective. The best way to 🎩is to 🎩

WHAT is this pull request doing?

  • Adds the focus ring where necessary according to Figma
    image

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx: 
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@kyledurand kyledurand added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Feb 14, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2020 •
edited
Loading

🟡 This pull request modifies 2 files and might impact 53 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:
All files potentially affected (total: 53)
🎨 src/components/Button/Button.scss (total: 53)

Files potentially affected (total: 53)

🎨 src/styles/shared/_buttons.scss (total: 0)

Files potentially affected (total: 0)

kyledurand
kyledurand commented Feb 14, 2020
View reviewed changes
src/styles/shared/_buttons.scss
@@ -235,13 +236,16 @@
}

&:focus {
box-shadow: 0 0 0 border-width('thick') var(--p-border-secondary);
@include no-focus-ring;
box-shadow: 0 0 0 border-width('base') var(--p-border-secondary);
Copy link
Member Author

Choose a reason for hiding this comment

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

This was probably out of date and changed in figma, or an oversight. This is the proper border width

@danrosenthal
Copy link
Contributor

Does this include the pressed state for Button? I was under the impression that was the remaining outstanding change.

@kyledurand
Copy link
Member Author

Pressed states: https://screenshot.click/screencast_2020-02-18_09-32-32.mp4

Monochrome is very subtle in figma as well. Maybe we can do better there

@kyledurand kyledurand force-pushed the buttons_polish branch 3 times, most recently from c13be7e to 1a2c1e5 Compare February 19, 2020 18:57
danrosenthal
danrosenthal suggested changes Feb 20, 2020
View reviewed changes
Copy link
Contributor

@danrosenthal danrosenthal left a comment

Choose a reason for hiding this comment

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

A bunch of these updates to color variants are wrong. Be sure to check the latest variants here: https://github.com/Shopify/polaris-tokens/blob/76f897fb186b1976b762c9189f95d05f526ad328/src/configs/base.ts#L197.

In particular the interactive changes are wrong, and should be reverted. Not sure about the rest, but it would be worth giving these a good look through.

@kyledurand
Copy link
Member Author

A bunch of these updates to color variants are wrong

So weird, those colors weren't working locally for me. I think I may have to clear my yarn cache

@kyledurand kyledurand changed the title [Button] New design language 💅 [Button] New design language Feb 21, 2020
dleroux
dleroux reviewed Feb 21, 2020
View reviewed changes
src/styles/shared/_buttons.scss
}

&:active {
box-shadow: 0 0 0 border-width('base') var(--p-border-secondary);
background: var(--p-surface-pressed);
&::after {
box-shadow: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought... we end up overwriting things about the ring in a few places. It doesn't feel right because it's so far detached for the source. If we change the ring, this will just add tech debt because chances are it will be left behind. Adding more arguments to the focus-ring with defaults would allow us to keep all the focus ring CSS in 1 location.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I've been chatting with Alex about refactoring button css once the design language has been rolled out. I think that would be a good time to address things like this as we'll have a better idea of how the focus ring needs to be used

@dleroux dleroux self-requested a review February 21, 2020 20:42
dleroux
dleroux approved these changes Feb 21, 2020
View reviewed changes
Copy link
Contributor

@dleroux dleroux 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 🚢

@kyledurand kyledurand merged commit c66a5d8 into master Feb 21, 2020
@kyledurand kyledurand deleted the buttons_polish branch February 21, 2020 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dleroux dleroux dleroux approved these changes

@danrosenthal danrosenthal danrosenthal approved these changes

@BPScott BPScott Awaiting requested review from BPScott

@tmlayton tmlayton Awaiting requested review from tmlayton

Assignees
No one assigned
Labels
🤖Skip Changelog Causes CI to ignore changelog update check.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kyledurand @danrosenthal @BPScott @dleroux