[Select] consolidate se23 styles

[gwyneplaine]

WHY are these changes introduced?

Fixes #9963

WHAT is this pull request doing?

• Change control-height to be 32px
• Select consolidate se23 styles

How to 🎩

• Storybook
• Prod storybook

🎩 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

[gwyneplaine]

Value ends up being used in TextField, Select and ResourceList. The first two override the value to be 32px with the feature flag enabled so they are not affected by this change.

The third, ResourceList, doesn't override this style. However it appears its being used as a min-height value, and (from what I can see) doesn't result in any visual change to the ResourceList component.

Before:

After:

Additionally, insofar as the logic described in the following issue around variable heights. It would appear that Select leverages line-height and font-size to enforce a height of 32px on larger screens and 36px on smaller ones. So defaulting the min-height specified in the control-height mixin to the minimum of 32px appears to be the right thing to do. At least until we address / resolve the behaviour outlined in the above linked issue. (cc @jesstelford)

[sophschneider]

Not sure whats going on with Error but it has a double focus ring. Other than that the tophat looked good!

[gwyneplaine]

Not sure whats going on with Error but it has a double focus ring. Other than that the tophat looked good!

Looks like i didn't remove a focus-ring mixin invocation, fixed ty for the catch 🥇

[sophschneider]

hmm now the focus ring isn't working for any of the selects :( https://5d559397bae39100201eedc1-sxmaeqinrc.chromatic.com/?path=/story/all-components-select--all maybe this commit?

[sam-b-rose]

Looks good! Just noticed the same issue @sophschneider pointed out about the focus ring missing.

[gwyneplaine]

hmm now the focus ring isn't working for any of the selects :( https://5d559397bae39100201eedc1-sxmaeqinrc.chromatic.com/?path=/story/all-components-select--all maybe this commit?

Fixed, deleted what I thought was a duplicate selector, reverted the change 👍

[jesstelford]

Interesting that there's not a @include no-focus-ring; to remove here. At first I though that meant the @include focus-ring($style: 'focused'); should remain, but looking at the later code, a more specific selector which does include @include no-focus-ring; wins:

.error .Input:focus-visible ~ .Backdrop {
  @include focus-ring($style: 'focused');
}
/* vs ... */
html[class~="Polaris-Summer-Editions-2023"] .Input:focus-visible ~ .Backdrop {
  @include focus-ring($style: 'focused');
  @include no-focus-ring;                /* <--- WINNER */
}

[jesstelford]

Oooft, control-height() strikes again!

I think your change is a good one, but I also think it leads to some other changes we should be making...

The se23 styles were modified like:

   min-height: control-height();
+  #{$se23} & {
+    min-height: 32px;
+  }

Which I think is because control-height() == 36px even in se23. Semantically, we still want those min-heights to match the control height, but the control-height() function was outdated, so hard coding a pixel value was easier (eg; in Select)

Now that your PR has updated control-height(), I think we should replace the hard coded values with the semantically correct function call in Option, TextField, and Select.

Note that Breadcrumbs looks, at first glance, to use a different value (min-height: 28px), throwing a spanner in the works. But I'm pretty certain that code will fully go away (the .Breadcrumb class isn't used in summer editions), so we can skip that one 👍

[gwyneplaine]

Oooft, control-height() strikes again!

I think your change is a good one, but I also think it leads to some other changes we should be making...

The se23 styles were modified like:

   min-height: control-height();
+  #{$se23} & {
+    min-height: 32px;
+  }

Which I think is because control-height() == 36px even in se23. Semantically, we still want those min-heights to match the control height, but the control-height() function was outdated, so hard coding a pixel value was easier (eg; in Select)

Now that your PR has updated control-height(), I think we should replace the hard coded values with the semantically correct function call in Option, TextField, and Select.

Note that Breadcrumbs looks, at first glance, to use a different value (min-height: 28px), throwing a spanner in the works. But I'm pretty certain that code will fully go away (the .Breadcrumb class isn't used in summer editions), so we can skip that one 👍

@jesstelford I've already made that change in Select I'm inclined to make the change to TextField and Banner in a separate PR, as presently those components aren't affected since they're still behind an se23 flag anyways.

Let me know if I've misinterpreted the ask here 👍

[jesstelford]

Agreed! Let's do it in a seperate PR 🚢