Add quiet styling to S2 Picker #6812

yihuiliao · 2024-07-31T19:05:05Z

Closes

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

yihuiliao · 2024-07-31T19:06:29Z

packages/@react-spectrum/s2/src/style-utils.ts

@@ -113,7 +113,8 @@ export const fieldInput = () => ({
  contain: {
    // Only apply size containment if contain-intrinsic-width is supported.
    // In older browsers, this will fall back to the default browser intrinsic width.
-    '@supports (contain-intrinsic-width: 1px)': 'inline-size'
+    '@supports (contain-intrinsic-width: 1px)': 'inline-size',
+    isQuiet: 'none'


honestly not too sure about this but without it, there's too much spacing between the value text and the dropdown chevron which doesn't match the figma designs for isQuiet

i didn't notice any other changes to other components that also used fieldInput()

packages/@react-spectrum/s2/src/Picker.tsx

rspbot · 2024-07-31T19:23:43Z

Build successful! 🎉

packages/@react-spectrum/s2/src/Picker.tsx

rspbot · 2024-08-01T18:21:37Z

Build successful! 🎉

rspbot · 2024-08-01T18:25:22Z

Build successful! 🎉

LFDanLu

Not sure if it is expected or not, but I noticed that the label for a quiet Picker sizes itself with respect to the length of the text content in the picker, whereas the label in v3 seems to prefer having the label as a single line regardless of the picker's selected text: https://reactspectrum.blob.core.windows.net/reactspectrum/1dcc8705115364a2c2ead2ececae8883dd6e9d07/storybook/index.html?path=/story/picker--quiet&providerSwitcher-express=false. Not sure if this is important if we are only adding quiet for Tabs (if that is even the case, lemme know if I'm misremembering)

packages/@react-spectrum/s2/src/Picker.tsx

reidbarber

One minor nit: there might be slightly too much space between the invalid icon and the value in quiet

snowystinger · 2024-08-01T23:40:31Z

custom width menu isn't quite wide enough, not sure what's going on there

snowystinger · 2024-08-01T23:45:59Z

popover alignment seems off when setting align="end" it's too far left I think, might need that negative margin adjustment? or maybe just need to get rid of the current one?

yihuiliao · 2024-08-02T21:56:29Z

packages/@react-spectrum/s2/src/Field.tsx

@@ -80,9 +81,10 @@ function FieldLabel(props: FieldLabelProps, ref: DOMRef<HTMLLabelElement>) {
        contain: {
          labelPosition: {
            top: 'inline-size'
-          }
+          },
+          isQuiet: 'none'


adding this allows the label be on a single line regardless of the picker's selected text

I guess we'll have to figure out if we want this, might interfere with form layouts

yeah im open to either. for tabs, there isn't going to be a label anyway so this won't matter too much. i added this to mainly to match the v3 behavior. i don't really think isQuiet should be used in forms given that the other form components don't have isQuiet styles but if you're curious, this is what it looks like having an isQuiet Picker in a form

for the second image, i had to apply a width to the form, otherwise, the text would overflow

yihuiliao · 2024-08-02T21:59:47Z

One minor nit: there might be slightly too much space between the invalid icon and the value in quiet

hmm yeah, it seems like it's because we add marginStart to the FieldErrorIcon. honestly, i'm not sure why we added this but after some digging it's been here since we created the component. maybe someone else knows?

react-spectrum/packages/@react-spectrum/s2/src/Field.tsx

Lines 329 to 330 in 617ad2e

    
           marginStart: 'text-to-visual', 
        
           marginEnd: fontRelative(-2),

rspbot · 2024-08-02T22:13:16Z

Build successful! 🎉

yihuiliao · 2024-08-02T23:12:54Z

custom width menu isn't quite wide enough, not sure what's going on there

actually, what is the expected behavior for the width of a popover when a custom width is applied for quiet pickers?

after i pushed the latest code, the width of the popover is not the same as the applied custom width because i had to add "fit-content" to the width of the button for quiet pickers to prevent the focus ring from extending past the dropdown chevron. as a result, the popover will always be the width of the trigger/button (unless the width of the trigger is less than the min width)

i tried it out in v3 and seems like if you apply any custom width to a quiet picker, the width of the popover will always be whatever the width of the trigger is, so S2 now matches the behavior of V3 but is this what we want?

yihuiliao · 2024-08-02T23:29:00Z

popover alignment seems off when setting align="end" it's too far left I think, might need that negative margin adjustment? or maybe just need to get rid of the current one?

it looks fine to me? or maybe im misunderstanding something but it looks to me like the popover is aligning to the end of the trigger. maybe my eyes just aren't picking it up...

LFDanLu · 2024-08-03T00:05:33Z

hmm yeah, it seems like it's because we add marginStart to the FieldErrorIcon. honestly, i'm not sure why we added this but after some digging it's been here since we created the component. maybe someone else knows?

Isn't that from the designs for spacing between text value to icon?

i tried it out in v3 and seems like if you apply any custom width to a quiet picker, the width of the popover will always be whatever the width of the trigger is, so S2 now matches the behavior of V3 but is this what we want?

Thats probably fine for now IMO, especially since it will just be for tabs for now and thus just always the min popover width

snowystinger · 2024-08-05T05:01:35Z

I mean the align start actually doesn't align with the start of the input, instead it's pushed further left. But align end is exactly lined up, it isn't pushed further right.

rspbot · 2024-08-06T02:09:27Z

Build successful! 🎉

snowystinger · 2024-08-06T03:01:21Z

packages/@react-spectrum/s2/src/Field.tsx

@@ -80,9 +81,10 @@ function FieldLabel(props: FieldLabelProps, ref: DOMRef<HTMLLabelElement>) {
        contain: {
          labelPosition: {
            top: 'inline-size'
-          }
+          },
+          isQuiet: 'none'


I guess we'll have to figure out if we want this, might interfere with form layouts

packages/@react-spectrum/s2/src/Picker.tsx

rspbot · 2024-08-06T18:31:54Z

Build successful! 🎉

snowystinger · 2024-08-06T21:33:50Z

I mean the align start actually doesn't align with the start of the input, instead it's pushed further left. But align end is exactly lined up, it isn't pushed further right.

Ah I see this was actually the behavior in v3 as well. I just checked the storybook for it. I think it's ok to leave it how you had it because I think we want that slight offset at the start. We can handle the align end case later. Would you mind opening a discussion with Spectrum about it?

Sorry for the back and forth

rspbot · 2024-08-07T18:49:32Z

Build successful! 🎉

rspbot · 2024-08-07T23:36:20Z

Build successful! 🎉

rspbot · 2024-08-08T00:28:13Z

Build successful! 🎉

rspbot · 2024-08-08T00:29:41Z

Build successful! 🎉

packages/@react-spectrum/s2/src/Picker.tsx

rspbot · 2024-08-08T16:41:25Z

Build successful! 🎉

rspbot · 2024-08-09T03:00:54Z

Build successful! 🎉

rspbot · 2024-08-09T16:36:45Z

Build successful! 🎉

add quiet styling

86faca7

yihuiliao commented Jul 31, 2024

View reviewed changes

packages/@react-spectrum/s2/src/Picker.tsx Show resolved Hide resolved

remove spacing

959cf4e

yihuiliao marked this pull request as ready for review July 31, 2024 19:25

snowystinger reviewed Aug 1, 2024

View reviewed changes

packages/@react-spectrum/s2/src/Picker.tsx Outdated Show resolved Hide resolved

packages/@react-spectrum/s2/src/Picker.tsx Outdated Show resolved Hide resolved

packages/@react-spectrum/s2/src/Picker.tsx Show resolved Hide resolved

yihuiliao added 2 commits August 1, 2024 11:05

fix spacing

f639f5d

simplify outline style

8c97dce

LFDanLu reviewed Aug 1, 2024

View reviewed changes

packages/@react-spectrum/s2/src/Picker.tsx Outdated Show resolved Hide resolved

packages/@react-spectrum/s2/src/Picker.tsx Outdated Show resolved Hide resolved

reidbarber previously approved these changes Aug 1, 2024

View reviewed changes

have label on a single line

35a7867

yihuiliao dismissed reidbarber’s stale review via 35a7867 August 2, 2024 21:54

yihuiliao commented Aug 2, 2024

View reviewed changes

adobe deleted a comment from rspbot Aug 2, 2024

long content in quiet picker to truncate in fixed width container

9783d79

snowystinger reviewed Aug 6, 2024

View reviewed changes

small fixes

a7a8a93

offset dropdown when align is start

da98c75

adobe deleted a comment from rspbot Aug 7, 2024

fix width of quiet picker dropdown

5e4042c

yihuiliao added 2 commits August 7, 2024 17:13

remove unused import

d39a3cf

remove another unused import

3ddfae2

snowystinger reviewed Aug 8, 2024

View reviewed changes

packages/@react-spectrum/s2/src/Picker.tsx Outdated Show resolved Hide resolved

update offset

4e27ddf

adobe deleted a comment from rspbot Aug 8, 2024

Merge branch 'main' into s2-picker-quiet

d4e9866

reidbarber approved these changes Aug 8, 2024

View reviewed changes

snowystinger approved these changes Aug 9, 2024

View reviewed changes

Merge branch 'main' into s2-picker-quiet

0c41025

akyllus approved these changes Aug 9, 2024

View reviewed changes

Merge branch 'main' into s2-picker-quiet

336182d

yihuiliao merged commit f56a787 into main Aug 9, 2024
29 checks passed

yihuiliao deleted the s2-picker-quiet branch August 9, 2024 17:08

dannify added the needs translations label Aug 15, 2024

Add quiet styling to S2 Picker #6812

Add quiet styling to S2 Picker #6812

Uh oh!

Conversation

yihuiliao commented Jul 31, 2024

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

yihuiliao Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rspbot commented Jul 31, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rspbot commented Aug 1, 2024

Uh oh!

rspbot commented Aug 1, 2024

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

reidbarber left a comment

Choose a reason for hiding this comment

Uh oh!

snowystinger commented Aug 1, 2024

Uh oh!

snowystinger commented Aug 1, 2024

Uh oh!

yihuiliao Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

snowystinger Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

yihuiliao Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

yihuiliao commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rspbot commented Aug 2, 2024

Uh oh!

yihuiliao commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yihuiliao commented Aug 2, 2024

Uh oh!

LFDanLu commented Aug 3, 2024

Uh oh!

snowystinger commented Aug 5, 2024

Uh oh!

rspbot commented Aug 6, 2024

Uh oh!

snowystinger Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rspbot commented Aug 6, 2024

Uh oh!

snowystinger commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rspbot commented Aug 7, 2024

Uh oh!

rspbot commented Aug 7, 2024

Uh oh!

rspbot commented Aug 8, 2024

Uh oh!

rspbot commented Aug 8, 2024

Uh oh!

Uh oh!

rspbot commented Aug 8, 2024

yihuiliao Jul 31, 2024 •

edited

Loading

yihuiliao commented Aug 2, 2024 •

edited

Loading

yihuiliao commented Aug 2, 2024 •

edited

Loading

snowystinger commented Aug 6, 2024 •

edited

Loading