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

Fix select knob default selection with array #7568

Merged
merged 2 commits into from
Jul 26, 2019
Merged

Fix select knob default selection with array #7568

merged 2 commits into from
Jul 26, 2019

Conversation

elliotlarson
Copy link
Contributor

@elliotlarson elliotlarson commented Jul 26, 2019

Issue: #7569

What I did

When using a select knob with arrays as option values, the default value was not getting set properly on the select list. The code was comparing two arrays with ===, which is always false.

I updated the code to stringify the arrays for doing the comparison.

How to test

There was not an existing test for the select knob that I could augment/add onto for this edge case. However, this is a pretty small update. I did try it out locally and it worked (manual test).

@vercel
Copy link

vercel bot commented Jul 26, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-fork-elliotlarson-next.storybook.now.sh

When using a select knob with arrays as option values, the selected key for the default value was not getting set properly.  The code was comparing two arrays with ===, which is always false.  This stringifies the arrays and then does the comparison.
Copy link
Member

@kroeder kroeder left a comment

Choose a reason for hiding this comment

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

I found a good place for a test 🙂

Although, I'm not sure I test this correctly 🤔
Even though the component itself has the correct value does this ensure that it is actually selected?

@ndelangen can you help me out here? I don't know much about enzyme nor react components

PR LGTM

@elliotlarson
Copy link
Contributor Author

@kroeder Thank you for adding a spec! I usually put my specs side by side with the implementation files. Somehow I totally missed the __tests__ directory 🤦‍♂️.

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.

2 participants