-
Notifications
You must be signed in to change notification settings - Fork 25
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
refactor(storybook): migrate to v7 - part 2 #2691
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
packages/components/pagination/src/page-navigator/page-navigator.stories.tsx
Outdated
Show resolved
Hide resolved
} | ||
}; | ||
|
||
const meta = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This story is not working correctly locally for me.
I'd like to have a sync about it. 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thank you, I'll schedule a meeting. I just wonder, it any different from https://uikit.commercetools.com/?path=/story/components-pagination--pagesizeselector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I found out is that using inline: false
will make the controls to not work.
Perhaps we can wrap the story in a component which applies that height.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I updated the story to use this parameter. However, I noticed a down side of using it. It works well in the Docs
page and the story page, but it doesn't work in the "README" page. That is why I removed the <Canvas of={PaginationStories.Default} />
from the pagination's readme.mdx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the docs, it seems we can adjust the height of the story the Canvas
component renders.
Testing it locally, this example seems to work:
<Canvas of={PaginationStories.Default} story={{ height: '560px' }}/>
The downside is that I had to use a high value for the height, but I think it's a fair compromise right now and maybe can take a deeper look in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
I'm encountering issues regardless of the set height. There are ongoing discussions in the Storybook repo related to the challenges with fixed positioning.
storybookjs/storybook#23586
storybookjs/storybook#23699
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe then we can just add a wrapper for the component in the story?
packages/components/primary-action-dropdown/src/primary-action-dropdown.stories.tsx
Show resolved
Hide resolved
…own, stamp to storybook v7
035b0a5
to
5cf843e
Compare
<Canvas> | ||
<Story of={PaginationStories.Default} height="100vh" /> | ||
</Canvas> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the only way I figured out to make this story work on the docs page.
type Story = StoryObj<typeof meta>; | ||
|
||
export const Default: Story = { | ||
// @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering why do we need this ignore directive.
I know this is a deprecated component, but since we want to keep the story, maybe we can just add its two required props.
argTypes: { | ||
tone: { | ||
control: 'select', | ||
options: ['primary', 'inverted', 'secondary'], | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we agree on having the args type definitions at the meta level?
argTypes: { | ||
...hideControls(['tone', 'children', 'icon', 'label']), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we agree on having the args type definitions at the meta level?
@CarlosCortizasCT argTypes at the meta level and link button story updated in 944daa1 |
Summary
Description