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.

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

[Storybook] Create Stories for EuiSideNav #7174

Merged
merged 9 commits into from
Sep 22, 2023

Conversation

breehall
Copy link
Contributor

@breehall breehall commented Sep 8, 2023

PR 1/2 for #7176

Summary

This story creates a Storybook playground and example for EuiSideNav. The playground is a kitchen sink example meant to demonstrate the nesting capability, mobile view, and truncation for EuiSideNav. The Header story is meant to provide a basic playground dedicated to EuiSideNavHeaderProps.

QA

@breehall breehall marked this pull request as ready for review September 9, 2023 22:12
@breehall breehall mentioned this pull request Sep 11, 2023
2 tasks
@cee-chen
Copy link
Member

cee-chen commented Sep 12, 2023

hmm, not sure why tests failed...

@cee-chen
Copy link
Member

buildkite test this

};

const StatefulSideNav = (props: Partial<EuiSideNavProps>) => {
const [isSideNavOpenOnMobile, setIsSideNavOpenOnMobile] = useState(false);
Copy link
Member

Choose a reason for hiding this comment

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

This should inherit from the storybook controls on initial mount/render

Suggested change
const [isSideNavOpenOnMobile, setIsSideNavOpenOnMobile] = useState(false);
const [isSideNavOpenOnMobile, setIsSideNavOpenOnMobile] = useState(props.isOpenOnMobile);

render: ({ ...args }) => <StatefulSideNav {...args} />,
};

export const SideNavHeader: Story = {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not totally clear on why header props need their own story. The heading props are equally as configurable in the playground story as they are in this story. Am I missing something?

isOpenOnMobile: false,
truncate: false,
},
render: ({ ...args }) => <StatefulSideNav {...args} />,
Copy link
Member

@cee-chen cee-chen Sep 13, 2023

Choose a reason for hiding this comment

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

I'm not a super huge fan of the stateful side nav being the default playground story. I'd much rather have the following stories:

  1. Non-stateful playground with items set via args.items, not via render, that allows developers to configure the item array/object via storybook controls. Using the items in the current stateful component as a static array would be fine, although I would not have stateful isSelected behavior. You can have 1 item with a static isSelected: true property just to show the visual behavior of it, but it doesn't need to be controlled via state.

  2. Story with stateful isOpenOnMobile / toggleOpenOnMobile. Note that the control for toggleOpenOnMobile should be set to { control: false } so that it can't be override in the control panel. I would only show those 2 props and mobileBreakpoints and mobileTitle on this story to keep the focus on mobile nav QA. I would also tweak the parameters of the storybook to start in small mobile view (https://storybook.js.org/docs/react/essentials/viewport)

  3. Story with custom renderItem callback

@breehall breehall requested a review from a team as a code owner September 22, 2023 19:08
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

🎉 Bree and I did a sync PR review this to streamline the stories a bit more

@breehall breehall enabled auto-merge (squash) September 22, 2023 19:53
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@breehall breehall merged commit 5d3838b into elastic:main Sep 22, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants