Creating chromatic baseline for fixing combobox to use useFormProps by ktabors · Pull Request #2334 · adobe/react-spectrum

ktabors · 2021-09-14T23:06:27Z

Closes #2323

Chromatic updates for #2351
Adding more from components to form and provider storybook and chromatic.

✅ 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:

Review the form stories
Review the provider stories

Run chromatic

🧢 Your Project:

RSP

…ybook/chromatic

ktabors

Need to review chromatic before this PR is ready.

ktabors · 2021-09-14T23:07:26Z

        <Checkbox value="cats">Cats</Checkbox>
        <Checkbox value="dragons">Dragons</Checkbox>
      </CheckboxGroup>
+      <ColorField label="Primary Color" />


May I refactor this to be one component of each type and in alphabetical order? Ditto the Provider update below.

I mean, it's already mangling the chromatic story, so, i guess? Do be careful, form chromatic stories were already close to their maximum height
It falls into the same category as the other chromatic PR you have, we just need to do it in three steps very carefully
first run chromatic on main, make sure it's all green. then run it on this branch, make sure we're ok with everything. merge it, then run chromatic once more on main so we have the right baseline moving forward

This sounds good. I was hoping that adding them at the end wouldn't cause too much chaos, but after reviewing, adding this many components makes the diff basically useless. :(

How do I tell what the max height is? Should the form stories be separated by categories like inputs and dropdowns?

ktabors · 2021-09-14T23:09:37Z

  .add(
    'custom theme',
    () => render({theme: THEME})
+  )


Should this file be refactored to use the new syntax?

at some point yes, if it's not hard you can do it here, if it takes more than a few min, maybe do it on its own later

snowystinger · 2021-09-15T00:30:50Z

 }

 function TextFieldBase(props: TextFieldBaseProps, ref: Ref<TextFieldRef>) {
-  props = useProviderProps(props);


why was useProviderProps removed here but not in SearchWithin? or should it be the other way around?

In my exploration I found having useProviderProps here did nothing. And it was needed in SearchWithin for the props like isQuiet, isRequired, isDisabled, etc. It is needed in any components that inherit those from the Provider. I was annoyed that I couldn't move useProviderProps to Field too.

adobe-bot · 2021-09-15T01:37:09Z

Build successful! 🎉

snowystinger

So it's occurred to me, we're doing code changes and chromatic changes together in this PR. They need to be split since they are modifications that will cause us to not get anything useful out of chromatic if kept together. Lets get the chromatic modifications out into their own PR, we'll merge that one, then we can merge the refactor of useFormProps

same plan as we did for the locale updates you were doing

ktabors · 2021-09-17T22:14:51Z

So it's occurred to me, we're doing code changes and chromatic changes together in this PR. They need to be split since they are modifications that will cause us to not get anything useful out of chromatic if kept together. Lets get the chromatic modifications out into their own PR, we'll merge that one, then we can merge the refactor of useFormProps

same plan as we did for the locale updates you were doing

#2351 has the code change, updating this to be chromatic and story updates

adobe-bot · 2021-09-17T22:49:26Z

Build successful! 🎉

LFDanLu

LGTM, should take special care when approving the chromatic diffs/new baseline for this however since this changes the order of the components in these stories

dannify · 2021-09-21T19:13:29Z

Can we keep the order of the form elements as they were before to minimize the visual diff for this?

ktabors · 2021-09-21T20:26:30Z

Can we keep the order of the form elements as they were before to minimize the visual diff for this?

That was my plan, but I found that by adding components it makes keeping the order pointless. The form width is different with these other components and after the first iteration, all the variations clobber each other because the height of each form is longer. I found more value in them being alphabetical to make sure we have all the ones we want.

dannify · 2021-09-21T20:29:10Z

ok sure.

reidbarber

LGTM

adobe-bot · 2021-09-21T21:00:20Z

Build successful! 🎉

* main: Rename cards packages -> card Bump theme packages add rest of style props to action menu (#2374) Publish Add Autocomplete package (#2371) CardView followup patch (#2362) Creating chromatic baseline for fixing combobox to use useFormProps (#2334) Correcting Arabic translations of Loading and Loading more (#2359)

fixing combobox to use useFormProps and adding more component to stor…

371b416

…ybook/chromatic

ktabors commented Sep 14, 2021

View reviewed changes

updating package.json

7dbdef0

snowystinger reviewed Sep 15, 2021

View reviewed changes

ktabors marked this pull request as ready for review September 15, 2021 00:34

ktabors requested review from dannify and devongovett as code owners September 15, 2021 00:34

removed colorfield and slider from form and provider stories

87d4256

reordering chromatic form and provider components

bb12d5a

snowystinger reviewed Sep 17, 2021

View reviewed changes

ktabors added 2 commits September 17, 2021 14:57

alphabetizing form and provider stories

695ca0b

Merge branch 'main' into field_formprops

78263c9

ktabors mentioned this pull request Sep 17, 2021

fixing combobox to use useFormProps and related component refactors #2351

Merged

5 tasks

ktabors added 2 commits September 17, 2021 15:21

removing the code changes to make this only chromatic and story updates

5d4b2a9

lint cleanups

98edead

ktabors changed the title ~~fixing combobox to use useFormProps~~ Creating chromatic baseline for fixing combobox to use useFormProps Sep 21, 2021

LFDanLu approved these changes Sep 21, 2021

View reviewed changes

reidbarber approved these changes Sep 21, 2021

View reviewed changes

Merge branch 'main' into field_formprops

bfc687e

ktabors merged commit 9ac0db3 into main Sep 21, 2021

ktabors deleted the field_formprops branch September 21, 2021 21:28

Conversation

ktabors commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

ktabors left a comment

Choose a reason for hiding this comment

Uh oh!

ktabors Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

snowystinger Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

ktabors Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ktabors Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

snowystinger Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

snowystinger Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

ktabors Sep 15, 2021 • 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!

adobe-bot commented Sep 15, 2021

Uh oh!

snowystinger left a comment

Choose a reason for hiding this comment

Uh oh!

ktabors commented Sep 17, 2021

Uh oh!

adobe-bot commented Sep 17, 2021

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

dannify commented Sep 21, 2021

Uh oh!

ktabors commented Sep 21, 2021

Uh oh!

dannify commented Sep 21, 2021

Uh oh!

reidbarber left a comment

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Sep 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ktabors commented Sep 14, 2021 •

edited

Loading

ktabors Sep 15, 2021 •

edited

Loading