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

docs(721): update select component scenarios - storybook #807

Merged
merged 9 commits into from
Apr 12, 2023
Merged

docs(721): update select component scenarios - storybook #807

merged 9 commits into from
Apr 12, 2023

Conversation

JohnTParsons
Copy link
Collaborator

closes #721

What

  1. Background - select component scenarios needed updating
  2. What did you do - updated them
  3. What does the reviewers should expect - new/updated storybook stories

I have done:

  • Written unit tests against changes
  • Written functional tests against the component and/or NewsKit site
  • Updated relevant documentation

I have tested manually:

  • The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
  • The screen reader reads and flows through the elements as expected.
  • There are no new errors in the browser console coming from this PR.
  • When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
  • The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

@github-actions github-actions bot added the docs Improvements or additions to documentation label Apr 5, 2023
@JohnTParsons JohnTParsons mentioned this pull request Apr 5, 2023
Closed
@pp-serviceaccount
Copy link
Collaborator

You can preview these changes on:
Documentation Site: http://ncu-newskit-docs-pr.s3-website-eu-west-1.amazonaws.com/721-select-storybook
Storybook: http://ncu-newskit-docs-pr.s3-website-eu-west-1.amazonaws.com/721-select-storybook/storybook

@JohnTParsons JohnTParsons added ready for review Please assist in getting this reviewed and removed Requires design review labels Apr 6, 2023
mutebg
mutebg reviewed Apr 10, 2023
View reviewed changes
src/select/__tests__/select.stories.tsx Outdated
</>
export const StorySelectDefault = () => (
<StorybookPage>
<Block>
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need this block here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, if I replace Block with StorybookCase. Change pushed

src/select/__tests__/select.stories.tsx Outdated
}}
>
export const StorySelectSize = () => (
<StorybookPage columns="1fr 1fr 1fr">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we leave this to defaults columns? This will not look good on mobile screens

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Change pushed

src/select/__tests__/select.stories.tsx Outdated
export const StorySelectSize = () => (
<StorybookPage columns="1fr 1fr 1fr">
<StorybookCase title="Small">
<Block>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with this block, don't thin is needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Change pushed

src/select/__tests__/select.stories.tsx Outdated
stylePreset: 'labelOverrides',
}}
export const StorySelectWidth = () => (
<StorybookPage columns="auto auto">
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should just use default columns most of the time ( they are 300px and auto flow ) even tho it does not match what's in Figma.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Change pushed

src/select/__tests__/select.stories.tsx Outdated
Comment on lines 380 to 384
{items.map(item => (
<SelectOption key={item} value={item}>
{item}
</SelectOption>
))}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we use this many times, what do you think of storing this in a const and re-using it in stories?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Change pushed

@JohnTParsons JohnTParsons marked this pull request as ready for review April 11, 2023 10:14
mutebg
mutebg reviewed Apr 12, 2023
View reviewed changes
Copy link
Contributor

@mutebg mutebg left a comment

Choose a reason for hiding this comment

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

These style presets are not used anywhere as far I can see but are defined in the theme overrides: selectOptionCustom, labelOverrides, assistiveTextOverrides, customOutlineColor, customOutlineStyle, customOutlineWidth, customOutlineOffset

src/select/__tests__/select.stories.tsx Outdated
</Container>
</>
export const StorySelectHeight = () => (
<StorybookPage columns="auto auto auto">
Copy link
Contributor

Choose a reason for hiding this comment

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

you have few more places with columns that might be worth checking if they are needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have removed all but one now

src/select/__tests__/select.stories.tsx
paddingBlock: 'space050',
paddingInline: 'space050',
typographyPreset: 'utilityButton010',
stylePreset: 'selectOptionCustom',
Copy link
Contributor

Choose a reason for hiding this comment

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

selectOptionCustom is still defined in the theme above but is not used any more, can you update the theme.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Change pushed

@JohnTParsons
Copy link
Collaborator Author

These style presets are not used anywhere as far I can see but are defined in the theme overrides: selectOptionCustom, labelOverrides, assistiveTextOverrides, customOutlineColor, customOutlineStyle, customOutlineWidth, customOutlineOffset

Change pushed

@JohnTParsons JohnTParsons merged commit f3758f1 into main Apr 12, 2023
@JohnTParsons JohnTParsons deleted the docs/721-select-storybook branch April 12, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mutebg mutebg mutebg approved these changes

@Vanals Vanals Vanals approved these changes

Assignees
No one assigned
Labels
docs Improvements or additions to documentation ready for review Please assist in getting this reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update Select component scenarios - Storybook
4 participants
@JohnTParsons @pp-serviceaccount @mutebg @Vanals