Skip to content

[Checkbox] consolidates se23 styles and logic #10015

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
Merged
merged 2 commits into from
Aug 17, 2023
Merged

[Checkbox] consolidates se23 styles and logic #10015

merged 2 commits into from
Aug 17, 2023

Conversation

@gwyneplaine
Copy link
Contributor

@gwyneplaine gwyneplaine commented Aug 11, 2023 •
edited
Loading

WHY are these changes introduced?

Fixes #9927

WHAT is this pull request doing?

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>
  );
}
  • Consolidate se23 logic
  • Consolidate se23 styles
  • Consolidate se23 logic in control-backdrop mixin

🎩 checklist

@gwyneplaine gwyneplaine added 🤖Skip Changelog Causes CI to ignore changelog update check. 🤖Skip Comment Check Skip the migrator comment CI check labels Aug 14, 2023
gwyneplaine
gwyneplaine commented Aug 14, 2023
View reviewed changes
polaris-react/src/components/Checkbox/Checkbox.scss
// stylelint-enable declaration-no-important
}
box-shadow: inset 0 0 0 var(--p-space-3)
var(--p-color-bg-critical-strong-active);
Copy link
Contributor Author

@gwyneplaine gwyneplaine Aug 14, 2023

Choose a reason for hiding this comment

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

Not sure why this !important was here initially, but correct specificity seems to be applied on removal.

@gwyneplaine
Copy link
Contributor Author

gwyneplaine commented Aug 15, 2023

Draft until #9928 is merged

@sam-b-rose sam-b-rose force-pushed the next branch 4 times, most recently from 5d459f8 to 700a72f Compare August 15, 2023 23:10
@gwyneplaine gwyneplaine marked this pull request as ready for review August 15, 2023 23:41
jesstelford
jesstelford reviewed Aug 16, 2023
View reviewed changes
jesstelford
jesstelford reviewed Aug 16, 2023
View reviewed changes
polaris-react/src/components/Checkbox/Checkbox.scss
bottom: calc(-1 * var(--p-space-05));
right: calc(-1 * var(--p-space-05));
/* stylelint-disable-next-line -- se23 override */
transform: none !important;
Copy link
Contributor

@jesstelford jesstelford Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

🤔 Hmmm, the !mportant here implies there was another style rule overriding the transform: none even though the #{$se23} & { selector was more specific (possibly the below .animated selector? But if so, why use !important instead of just disabling it within .animated { #{$se23} & {?)

By removing it now, there may still be a transform being applied from somewhere that we don't want... Can you confirm that's no longer the case?

Copy link
Contributor Author

@gwyneplaine gwyneplaine Aug 16, 2023

Choose a reason for hiding this comment

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

@jesstelford Cursory search and testing, looks to be an unnecessary !important in the first place.
Here's what its overriding in the prod storybook
Screenshot 2023-08-16 at 3 33 11 pm

Don't see any issues in the PR storybook AFAIK. This should be safe to merge

@shopify-github-actions-access shopify-github-actions-access bot mentioned this pull request Aug 16, 2023
Closed
jesstelford
jesstelford reviewed Aug 16, 2023
View reviewed changes
@gwyneplaine gwyneplaine force-pushed the v12/9927 branch 2 times, most recently from 726654e to fec7289 Compare August 16, 2023 05:43
laurkim
laurkim approved these changes Aug 16, 2023
View reviewed changes
Copy link
Contributor

@laurkim laurkim left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Padding is a bit off for xs and sm breakpoints for the Error story when comparing it to prod storybook but all other examples at breakpoints lgtm 👍

@gwyneplaine
Copy link
Contributor Author

gwyneplaine commented Aug 17, 2023

Padding is a bit off for xs and sm breakpoints for the Error story when comparing it to prod storybook but all other examples at breakpoints lgtm 👍

Looks like an inadvertent bug fix, previously Choice wasn't correctly applying changes to the --pc-choice-size variable in its calculation of padding in the description element.

Screenshot 2023-08-17 at 11 11 56 am

@gwyneplaine gwyneplaine merged commit 045566d into next Aug 17, 2023
@gwyneplaine gwyneplaine deleted the v12/9927 branch August 17, 2023 01:23
kyledurand pushed a commit that referenced this pull request Aug 17, 2023
@gwyneplaine @kyledurand
[Checkbox] consolidates se23 styles and logic (#10015)
80671c9 
<!--
  ☝️How to write a good PR title:
- Prefix it with [ComponentName] (if applicable), for example: [Button]
  - Start with a verb, for example: Add, Delete, Improve, Fix…
  - Give as much context as necessary and as little as possible
  - Prefix it with [WIP] while it’s a work in progress
-->

### WHY are these changes introduced?

Fixes #9927 <!-- link to issue if one exists -->

<!--
  Context about the problem that’s being addressed.
-->

### WHAT is this pull request doing?

<!--
  Summary of the changes committed.

Before / after screenshots are appreciated for UI changes. Make sure to
include alt text that describes the screenshot.

If you include an animated gif showing your change, wrapping it in a
details tag is recommended. Gifs usually autoplay, which can cause
accessibility issues for people reviewing your PR:

    <details>
      <summary>Summary of your gif(s)</summary>
      <img src="..." alt="Description of what the gif shows">
    </details>
-->

<!-- ℹ️ Delete the following for small / trivial changes -->

### How to 🎩

🖥 [Local development
instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development)
🗒 [General tophatting
guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md)
📄 [Changelog
guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog)

<!--
  Give as much information as needed to experiment with the component
  in the playground.
-->

<details>
<summary>Copy-paste this code in
<code>playground/Playground.tsx</code>:</summary>

```jsx
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>
  );
}
```

</details>

- Consolidate se23 logic
- Consolidate se23 styles
- Consolidate se23 logic in control-backdrop mixin 

### 🎩 checklist

- [ ] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [ ] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [ ] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [ ] Updated the component's `README.md` with documentation changes
- [ ] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide
sophschneider pushed a commit that referenced this pull request Sep 19, 2023
@gwyneplaine @sophschneider
[Checkbox] consolidates se23 styles and logic (#10015)
2e7b26e 
<!--
  ☝️How to write a good PR title:
- Prefix it with [ComponentName] (if applicable), for example: [Button]
  - Start with a verb, for example: Add, Delete, Improve, Fix…
  - Give as much context as necessary and as little as possible
  - Prefix it with [WIP] while it’s a work in progress
-->

### WHY are these changes introduced?

Fixes #9927 <!-- link to issue if one exists -->

<!--
  Context about the problem that’s being addressed.
-->

### WHAT is this pull request doing?

<!--
  Summary of the changes committed.

Before / after screenshots are appreciated for UI changes. Make sure to
include alt text that describes the screenshot.

If you include an animated gif showing your change, wrapping it in a
details tag is recommended. Gifs usually autoplay, which can cause
accessibility issues for people reviewing your PR:

    <details>
      <summary>Summary of your gif(s)</summary>
      <img src="..." alt="Description of what the gif shows">
    </details>
-->

<!-- ℹ️ Delete the following for small / trivial changes -->

### How to 🎩

🖥 [Local development
instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development)
🗒 [General tophatting
guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md)
📄 [Changelog
guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog)

<!--
  Give as much information as needed to experiment with the component
  in the playground.
-->

<details>
<summary>Copy-paste this code in
<code>playground/Playground.tsx</code>:</summary>

```jsx
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>
  );
}
```

</details>

- Consolidate se23 logic
- Consolidate se23 styles
- Consolidate se23 logic in control-backdrop mixin 

### 🎩 checklist

- [ ] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [ ] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [ ] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [ ] Updated the component's `README.md` with documentation changes
- [ ] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide
@sam-b-rose sam-b-rose mentioned this pull request Sep 19, 2023
Merged
AnnaCheba pushed a commit to AnnaCheba/polaris that referenced this pull request Apr 22, 2024
@gwyneplaine
[Checkbox] consolidates se23 styles and logic (Shopify#10015)
f4486f1 
<!--
  ☝️How to write a good PR title:
- Prefix it with [ComponentName] (if applicable), for example: [Button]
  - Start with a verb, for example: Add, Delete, Improve, Fix…
  - Give as much context as necessary and as little as possible
  - Prefix it with [WIP] while it’s a work in progress
-->

### WHY are these changes introduced?

Fixes Shopify#9927 <!-- link to issue if one exists -->

<!--
  Context about the problem that’s being addressed.
-->

### WHAT is this pull request doing?

<!--
  Summary of the changes committed.

Before / after screenshots are appreciated for UI changes. Make sure to
include alt text that describes the screenshot.

If you include an animated gif showing your change, wrapping it in a
details tag is recommended. Gifs usually autoplay, which can cause
accessibility issues for people reviewing your PR:

    <details>
      <summary>Summary of your gif(s)</summary>
      <img src="..." alt="Description of what the gif shows">
    </details>
-->

<!-- ℹ️ Delete the following for small / trivial changes -->

### How to 🎩

🖥 [Local development
instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development)
🗒 [General tophatting
guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md)
📄 [Changelog
guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog)

<!--
  Give as much information as needed to experiment with the component
  in the playground.
-->

<details>
<summary>Copy-paste this code in
<code>playground/Playground.tsx</code>:</summary>

```jsx
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>
  );
}
```

</details>

- Consolidate se23 logic
- Consolidate se23 styles
- Consolidate se23 logic in control-backdrop mixin 

### 🎩 checklist

- [ ] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [ ] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [ ] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [ ] Updated the component's `README.md` with documentation changes
- [ ] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@laurkim laurkim laurkim approved these changes

@sam-b-rose sam-b-rose Awaiting requested review from sam-b-rose

@sophschneider sophschneider Awaiting requested review from sophschneider

+1 more reviewer

@jesstelford jesstelford jesstelford approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

🤖Skip Changelog Causes CI to ignore changelog update check. 🤖Skip Comment Check Skip the migrator comment CI check

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gwyneplaine @jesstelford @laurkim