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

📖 [amp-list][storybook][amp] Added basic storybook stories for amp-list #27719

Merged
merged 3 commits into from
Apr 22, 2020

Conversation

krdwan
Copy link
Contributor

@krdwan krdwan commented Apr 13, 2020

Added two stories in storybook for amp-list

@krdwan
Copy link
Contributor Author

krdwan commented Apr 13, 2020

@wassgha A storybook story for amp-sidebar for your review. I was unable to add custom CSS, do you have any information on how to add CSS to storybook? If so, I may update this PR.

Comment on lines +17 to +18
// To do: how to add CSS
//import '!style-loader!css-loader!./Basic-styles.css';
Copy link
Contributor

Choose a reason for hiding this comment

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

See #27800

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll add a separate PR to include more use cases with CSS

Copy link
Contributor

Choose a reason for hiding this comment

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

The CSS pull request is approved.

Comment on lines 36 to 39
<p>
Use the buttons below to open / toggle the amp-sidebar. The sidebar
will be displayed with various navigational elements.
</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually examples should just be the component itself without any description. If we like we can use the storybook docs addon which allows for this style of inline examples. See https://github.com/storybookjs/storybook/tree/master/addons/docs#docspage
/cc @dvoytenko

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the input @wassgha . I am happy to remove the description or move the entire story into a storybook doc.

One thing to note is that it appears that some components do not work in isolation. For amp-list here, at a minimum a button outside of the component is needed to toggle the sidebar into view.

Sounds like we should establish a precedent on how we want to handle potential descriptions and other elements outside of the component in a story.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome! Descriptions moved to comments as we discussed in latest commit.

Copy link
Contributor

@dvoytenko dvoytenko left a comment

Choose a reason for hiding this comment

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

LGTM on my side, but pls complete the review with @wassgha

@wassgha wassgha merged commit d68d2cc into ampproject:master Apr 22, 2020
ldoroshe pushed a commit to ldoroshe/amphtml that referenced this pull request May 8, 2020
* Create basic stories for amp-sidebar in storybook

* Update amp-sidebar story to not have description in story, description moved to comments

* Re-kickoff travis
@krdwan krdwan changed the title 📖 Added basic storybook stories for amp-list 📖 [amp-list][storybook][amp] Added basic storybook stories for amp-list May 14, 2020
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.

None yet

5 participants