Skip to content

Conversation

dleroux
Copy link
Contributor

@dleroux dleroux commented Jan 29, 2020

WHY are these changes introduced?

Touches on:
https://github.com/Shopify/polaris-ux/issues/371
https://github.com/Shopify/polaris-ux/issues/382
https://github.com/Shopify/polaris-ux/issues/361

WHAT is this pull request doing?

For Button this fixes the focus state of outline monochrome buttons (Not .globalTheming hence the change log entry)
buttonAfter

For Split Button this fixes the focus ring
SplitButtonAfter

For Checkable Button this fixes the alignment of the checkbox which was off by a few pixels
bulkActionAfter

For ButtonGroup and BulkActions this fixes the focus ring
buttonGroupAfter

How to 🎩

  • Top hat using the playground:
  1. Button focus states with and without enabling theming.
  2. BulkActions and Button Group

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 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

@github-actions
Copy link
Contributor

github-actions bot commented Jan 29, 2020

🟡 This pull request modifies 8 files and might impact 69 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: 69)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

📄 documentation/Color system.md (total: 0)

Files potentially affected (total: 0)

🎨 src/components/Button/Button.scss (total: 53)

Files potentially affected (total: 53)

🎨 src/components/Checkbox/Checkbox.scss (total: 6)

Files potentially affected (total: 6)

🎨 src/components/RadioButton/RadioButton.scss (total: 2)

Files potentially affected (total: 2)

🎨 src/components/ResourceList/components/CheckableButton/CheckableButton.scss (total: 3)

Files potentially affected (total: 3)

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

Files potentially affected (total: 0)

🧩 src/utilities/theme/tokens.ts (total: 65)

Files potentially affected (total: 65)

@dleroux dleroux force-pushed the cs-checkable-button branch from ef3516c to e46a4e8 Compare January 29, 2020 15:21
|`--p-non-null-content`|`''`|
|`--p-choice-size`|`2rem`|
|`--p-choice-margin`|`0.1rem`|
|`--p-control-border-width`|`0.2rem`|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why this entire file changed but this is what actually changed

}
> :first-child .Button {
> :first-child .Button,
> :first-child .Button::after {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Easy peasy. I didn't bother adding the .globalTheming here because this doesn't impact the non-themable buttons.

}

&:focus {
box-shadow: 0 0 0 1px currentColor;
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 is actual bug fix for outline monochrome focus.

@@ -1,5 +1,4 @@
@import '../../styles/common';
$control-size: rem(16px);
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 wasn't being used 🤷‍♂

}

.RadioButton.globalTheming {
--control-border-width: #{rem(2px)};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this to tokens since it was was in 2 places

// padding to fix the bottom of letters being cutoff by overflow: hidden
padding: rem(1px) 0;
.globalTheming & {
margin-left: calc(#{$chekbox-label-margin} - var(--p-control-border-width));
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 was causing the checkbox to be misaligned

@dleroux dleroux force-pushed the cs-checkable-button branch from e761a84 to 7a5297d Compare January 29, 2020 15:44
@dleroux dleroux changed the title [wip][Checkable Button, Button and ButtonGroup, BulkAction] apply design language [Checkable Button, Button and ButtonGroup, BulkAction] apply design language Jan 29, 2020
@dleroux dleroux requested a review from kyledurand January 29, 2020 15:46
@dleroux dleroux force-pushed the cs-checkable-button branch 3 times, most recently from 61df2ce to e5ea832 Compare January 30, 2020 19:07
@dleroux dleroux force-pushed the cs-checkable-button branch from e5ea832 to f26b0d7 Compare February 3, 2020 18:37
@dleroux dleroux force-pushed the cs-checkable-button branch from f26b0d7 to fe6031a Compare February 3, 2020 18:53
Copy link
Member

@kyledurand kyledurand 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 👍

@dleroux dleroux merged commit 5c05b4d into master Feb 3, 2020
@dleroux dleroux deleted the cs-checkable-button branch February 3, 2020 19:02
@chloerice chloerice temporarily deployed to production February 3, 2020 21:43 Inactive
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.

4 participants