Skip to content

Conversation

@ktabors
Copy link
Member

@ktabors ktabors commented May 21, 2022

Part of the Storybook CSF 3.0 migration in issue 3056.

There are changes to Chromatic so we'll need to confirm the migrated stories didn't change and approve the new stories.

  • Run Chromatic and approve the new stories.

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

Goto storybook and review the new HelpText and Label stories. Confirm that via controls you can do everything we used to be able to do. There used to be many specific stories and now there are just a couple of general stories.

🧢 Your Project:

RSP

@adobe-bot
Copy link

Build successful! 🎉

@ktabors
Copy link
Member Author

ktabors commented Jun 3, 2022

Auditing the HelpText Chromatic stories. The following are stories not included and why. An example of validationState='valid' with no description but has an errorMessage doesn't reveal anything in Chromatic, we only test that via interacting with it. A isDisabled with validationState='invalid' and an errorMessage is a "valid" combination of props, but not something that would happen in a UI.

<TextField label="Password" {...props} />
);
}
export const ContainerWithTextAlignmentSet = {
Copy link
Member Author

Choose a reason for hiding this comment

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

This story was in storybook and since it wasn't interactive I moved it to Chromatic.

@ktabors ktabors marked this pull request as ready for review June 4, 2022 00:18
@adobe-bot
Copy link

Build successful! 🎉

Comment on lines 91 to 107
export const ContainerWithTextAlignmentSet = {
render: () => (
<Flex
direction="column"
gap="size-200"
UNSAFE_style={{
textAlign: 'center'
}}>
<TextField label="Password" description="Enter a single digit number." />
<TextField
label="Password 2"
errorMessage="Create a password with at least 8 characters."
validationState="invalid" />
</Flex>
),
name: 'container with text alignment set'
};
Copy link
Member

Choose a reason for hiding this comment

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

Let's break this into two and use decorators so that we don't need a custom renderer and we can just use args (I probably have some typos in this, but this is the general idea)

Suggested change
export const ContainerWithTextAlignmentSet = {
render: () => (
<Flex
direction="column"
gap="size-200"
UNSAFE_style={{
textAlign: 'center'
}}>
<TextField label="Password" description="Enter a single digit number." />
<TextField
label="Password 2"
errorMessage="Create a password with at least 8 characters."
validationState="invalid" />
</Flex>
),
name: 'container with text alignment set'
};
export const ContainerWithTextAlignmentSetDescription = {
args: {
label: 'Password'
description: 'Enter a single digit number'
},
name: 'textAlign center description',
decorator: [textAlignDecorator]
};
export const ContainerWithTextAlignmentSetError = {
args: {
label: 'Password'
errorMessage: 'Create a password with at least 8 characters.'
},
name: 'textAlign center errorMessage',
decorator: [textAlignDecorator]
};
function textAlignDecorator(Story) {
return (
<Flex
direction="column"
gap="size-200"
UNSAFE_style={{
textAlign: 'center'
}}>
<Story />
</Flex>
)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed we wanted two TextFields next to each other.

function TextFieldLabel(props: SpectrumLabelProps = {}) {
return (
<div style={{whiteSpace: 'nowrap'}}>
<Label {...props} for="test">
Copy link
Member

Choose a reason for hiding this comment

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

htmlFor not for

} as ComponentMeta<typeof Label>;

export let Default: LabelStory = {
render: (args) => <TextFieldLabel {...args} />
Copy link
Member

Choose a reason for hiding this comment

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

We can do a similar setup to the helptext so we don't need a custom render, we can use a decorator for these so args is just applied directly to the Label

errorMessage: 'Remove input.'
},
argTypes: {validationState: {control: {disable: true}}},
render: (props) => <TextFieldWithValidationState {...props} />
Copy link
Member

Choose a reason for hiding this comment

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

same comment
the reason for this is so that stories are actually about our component and types across stories are the same
this should make it easy to track controls as well

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried using instead of and I couldn't add the state this story uses. 👎

} as ComponentMeta<typeof Label>;

export let Default: LabelStory = {
render: (args) => <TextFieldLabel {...args} />
Copy link
Member

Choose a reason for hiding this comment

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

same thing

@ktabors ktabors changed the title Label and HelpText storybook CSF 3.0 CSF 3.0 Label and HelpText Jun 7, 2022
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

@ktabors ktabors merged commit 922dbe8 into main Jun 14, 2022
@ktabors ktabors deleted the csf3-label branch June 14, 2022 20:39
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.

5 participants