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

[Select] consolidate se23 styles #10081

Merged
merged 2 commits into from
Aug 18, 2023
Merged

[Select] consolidate se23 styles #10081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1e892d
Change control-height to be 32px
gwyneplaine Aug 16, 2023
0f16330
`Select` consolidate se23 styles
gwyneplaine Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 24 additions & 69 deletions polaris-react/src/components/Select/Select.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
display: none;
}

#{$se23} &:not(.disabled):not(.error) {
&:not(.disabled):not(.error) {
// stylelint-disable-next-line -- se23 overrides
&:hover .Backdrop {
border-color: var(--p-color-border-input-hover);
Expand Down Expand Up @@ -52,17 +52,10 @@

.Backdrop {
border-color: var(--p-color-border-disabled);

#{$se23} & {
background-color: var(--p-color-bg-transparent-disabled-experimental);
}
background-color: var(--p-color-bg-transparent-disabled-experimental);

&::before {
background-color: var(--p-color-bg-disabled);

#{$se23} & {
background-color: var(--p-color-bg-input);
}
background-color: var(--p-color-bg-input);
}

&:hover {
Expand All @@ -82,22 +75,15 @@
width: 100%;
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
min-height: control-height();
// stylelint-disable -- generated by polaris-migrator DO NOT COPY
padding: control-vertical-padding() var(--p-space-2)
control-vertical-padding() var(--p-space-3);
// stylelint-enable
padding: var(--p-space-1_5-experimental) var(--p-space-2)
var(--p-space-1_5-experimental) var(--p-space-3);

#{$se23} & {
min-height: 32px;
font-size: var(--p-font-size-200);
line-height: var(--p-font-line-height-3);
padding: var(--p-space-1_5-experimental) var(--p-space-2)
var(--p-space-1_5-experimental) var(--p-space-3);
font-size: var(--p-font-size-200);
line-height: var(--p-font-line-height-3);

@media #{$p-breakpoints-md-up} {
line-height: var(--p-font-line-height-2);
font-size: var(--p-font-size-80-experimental);
}
@media #{$p-breakpoints-md-up} {
line-height: var(--p-font-line-height-2);
font-size: var(--p-font-size-80-experimental);
}

@media #{$p-breakpoints-md-down} {
Expand All @@ -121,11 +107,7 @@
}

.Icon svg {
fill: var(--p-color-icon);

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

.Input {
Expand Down Expand Up @@ -161,33 +143,18 @@
right: 0;
bottom: 0;
left: 0;
border: var(--p-border-width-1) solid var(--p-color-border-input);
border-radius: var(--p-border-radius-1);
background-color: var(--p-color-bg);
box-shadow: var(--p-shadow-sm);
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($border-width: 1px);
border: var(--p-border-width-1-experimental) solid var(--p-color-border-input);
border-radius: var(--p-border-radius-2);
background-color: var(--p-color-bg-input);
// stylelint-disable-next-line order/properties-order -- 'position' needs to sit below focus-ring since it will be overwritten with relative when the focus ring style is 'base'
position: absolute;

#{$se23} & {
background-color: var(--p-color-bg-input);
border-radius: var(--p-border-radius-2);
border-width: var(--p-border-width-1-experimental);
box-shadow: none;
}
}

.error {
.Backdrop {
border-color: var(--p-color-border-critical);
border-color: var(--p-color-border-critical-strong-experimental);
background-color: var(--p-color-bg-critical-subdued);

#{$se23} & {
border-color: var(--p-color-border-critical-strong-experimental);
background-color: var(--p-color-bg-critical-subdued);
}

// stylelint-disable-next-line selector-max-class -- generated by polaris-migrator DO NOT COPY
&.hover,
&:hover {
Expand All @@ -199,32 +166,20 @@
// so that errors still have red borders.
// stylelint-disable-next-line selector-max-combinators, selector-max-specificity, selector-max-class -- generated by polaris-migrator DO NOT COPY
.Input:focus-visible ~ .Backdrop {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');

#{$se23} & {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 */
}

border-color: var(--p-color-border-critical-strong-experimental);
border-width: var(--p-border-width-2-experimental);
background-color: var(--p-color-bg-critical-subdued);
}
border-color: var(--p-color-border-critical-strong-experimental);
border-width: var(--p-border-width-2-experimental);
background-color: var(--p-color-bg-critical-subdued);
}
}

.Input:focus-visible {
~ .Backdrop {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');

#{$se23} & {
border-color: var(--p-color-border-input-active-experimental);
border-width: var(--p-border-width-2-experimental);
background-color: var(--p-color-bg-input-active-experimental);
// stylelint-disable-next-line -- se23
@include no-focus-ring;
outline: var(--p-border-width-2) solid
var(--p-color-border-interactive-focus);
outline-offset: var(--p-space-025);
}
border-color: var(--p-color-border-input-active-experimental);
border-width: var(--p-border-width-2-experimental);
background-color: var(--p-color-bg-input-active-experimental);
outline: var(--p-border-width-2) solid
var(--p-color-border-interactive-focus);
outline-offset: var(--p-space-025);
}
}

Expand Down
2 changes: 1 addition & 1 deletion polaris-react/src/styles/shared/_controls.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
@function control-height() {
@return 36px;
@return 32px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:
Screenshot 2023-08-16 at 5 27 55 pm

After:
Screenshot 2023-08-16 at 5 28 01 pm

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)

}

@function control-vertical-padding() {
Expand Down