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

[TextField] consolidate se23 styles and logic #10134

Merged
merged 2 commits into from
Aug 22, 2023
Merged

[TextField] consolidate se23 styles and logic #10134

merged 2 commits into from
Aug 22, 2023

Conversation

gwyneplaine
Copy link
Contributor

@gwyneplaine gwyneplaine commented Aug 21, 2023

WHY are these changes introduced?

Fixes #9972

WHAT is this pull request doing?

  • TextField consolidate se23 logic
  • TextField consolidate se23 styles

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

fill: var(--p-color-icon);

#{$se23} & {
fill: var(--p-color-icon-subdued);
Copy link
Contributor Author

@gwyneplaine gwyneplaine Aug 21, 2023

Choose a reason for hiding this comment

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

This was previously incorrectly overriding the svg fill value on disabled. Hence the storybook change.
See Figma

See Storybook changes

Copy link
Contributor

@laurkim laurkim left a comment

Choose a reason for hiding this comment

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

Overall looks great 💯

Small issues I noticed:

  • Prefix has incorrect line height/font size for xs/sm breakpoints
  • Borderless example in All storybook has incorrect line height for xs/sm breakpoints

@gwyneplaine
Copy link
Contributor Author

gwyneplaine commented Aug 21, 2023

  • Prefix has incorrect line height/font size for xs/sm breakpoints

Thanks for the catch on this one 🙏 , fixed 👍

  • Borderless example in All storybook has incorrect line height for xs/sm breakpoints

Looks like the $se23 .Input styles in prod are overriding non uplift .borderless styles. Not sure if this was intentional or a specificity regression during uplift.

Prod:
Screenshot 2023-08-22 at 9 44 13 am

This PR:
Screenshot 2023-08-22 at 9 43 51 am

I'm gonna remove the line-height declaration in .borderless such that it inherits line-height changes in .Input since that seems to be in the spirit of the se23 changes, but happy to revert in a subsequent PR @laurkim (cc @sarahill)

@@ -264,7 +216,6 @@ $spinner-icon-size: 12px;
.Input,
.Backdrop {
border: none;
line-height: var(--p-font-line-height-1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this line as it was already being overridden in pre-uplift due to specificity.
This particular line height declaration also doesn't seem to make sense in the context of the new DL, taking into account the se23 changes that have been made.

@gwyneplaine gwyneplaine merged commit 210f212 into next Aug 22, 2023
16 checks passed
@gwyneplaine gwyneplaine deleted the v12/9972 branch August 22, 2023 00:14
sophschneider pushed a commit that referenced this pull request Sep 19, 2023
<!--
  ☝️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 #9972 


### WHAT is this pull request doing?

- `TextField` consolidate se23 logic
- `TextField` consolidate se23 styles


### 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>

### 🎩 checklist

-
[Storybook](https://5d559397bae39100201eedc1-ijxwodqxyd.chromatic.com/?path=/story/all-components-textfield--all)
- [Prod
Storybook](https://storybook.polaris.shopify.com/?path=/story/all-components-textfield--all&globals=polarisSummerEditions2023:true)
AnnaCheba pushed a commit to AnnaCheba/polaris that referenced this pull request Apr 22, 2024
<!--
  ☝️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#9972 


### WHAT is this pull request doing?

- `TextField` consolidate se23 logic
- `TextField` consolidate se23 styles


### 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>

### 🎩 checklist

-
[Storybook](https://5d559397bae39100201eedc1-ijxwodqxyd.chromatic.com/?path=/story/all-components-textfield--all)
- [Prod
Storybook](https://storybook.polaris.shopify.com/?path=/story/all-components-textfield--all&globals=polarisSummerEditions2023:true)
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.

[TextField] Consolidate se23 logic and styles
2 participants