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

Add TreeSelect component to Storybook #20496

Merged

Conversation

ediamin
Copy link
Contributor

@ediamin ediamin commented Feb 27, 2020

Description

This PR add TreeSelect component to Storybook.

How has this been tested?

I've tested in my localhost running npm run storybook:dev.

Screenshots

Screenshot 2020-02-27 at 3 37 51 PM

Types of changes

  • Added new Storybook item

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

Copy link
Contributor

@brentswisher brentswisher left a comment

Choose a reason for hiding this comment

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

Thank you so much for your work on this @ediamin, it's looking good! 👍 I added a few notes below with some suggestions for minor improvements, let me know if you have any questions.

I also saw that it failed the storybook unit test, this usually just happens because the snapshot needs to be updated to include the new story. If you run npm run test-unit storybook/ it should take care of generating the new snapshot.

packages/components/src/tree-select/stories/index.js Outdated Show resolved Hide resolved
packages/components/src/tree-select/stories/index.js Outdated Show resolved Hide resolved
@ediamin
Copy link
Contributor Author

ediamin commented Mar 2, 2020

@brentswisher Thanks for the tips. I've followd you select-control story and update mine. Please review now.

Copy link
Contributor

@brentswisher brentswisher left a comment

Choose a reason for hiding this comment

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

This is all looking great! The only thing that is holding it up is it looks like the snapshot is out of date (Due to auto-incrementing ids, not anything you did) Do you think you can regenerate it and then it will be good to merge 👍

@ediamin ediamin force-pushed the add_treeselect_component_to_storybook branch from 92a704a to 3b08f48 Compare March 18, 2020 14:10
@ediamin
Copy link
Contributor Author

ediamin commented Mar 18, 2020

@brentswisher Updated the snapshot. Please review now.

Copy link
Contributor

@brentswisher brentswisher left a comment

Choose a reason for hiding this comment

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

Looks great to me, the failing e2e test in Travis looks unrelated. Thanks for all your work @ediamin! 👏

@ediamin
Copy link
Contributor Author

ediamin commented Mar 25, 2020

Hi @brentswisher, is there anything else I should do? Just curious, why the PR is still been open even after you approved.

@youknowriad
Copy link
Contributor

I guess we just forgot about it :P. It happens.

@youknowriad youknowriad merged commit 37995eb into WordPress:master Mar 25, 2020
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Mar 25, 2020
@brentswisher
Copy link
Contributor

@youknowriad thanks! @ediamin I'm really sorry, I had restarted the tests to see if I could get them to pass and forgot to come back and check! Thanks for staying on top of it and for the help!

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.

3 participants