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 support for <select multiple> #15

Merged
merged 4 commits into from
Feb 21, 2019
Merged

Add support for <select multiple> #15

merged 4 commits into from
Feb 21, 2019

Conversation

gregleveque
Copy link
Contributor

@gregleveque gregleveque commented Feb 13, 2019

Closes #13

@coveralls
Copy link

coveralls commented Feb 13, 2019

Pull Request Test Coverage Report for Build 79

  • 12 of 12 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 66: 0.0%
Covered Lines: 74
Relevant Lines: 74

💛 - Coveralls

Copy link
Owner

@wsmd wsmd left a comment

Choose a reason for hiding this comment

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

Nice work, @gregleveque! Thanks for taking the time to work on this PR!

I have a few suggestions:

I was initially thinking that a multiple select can be used like this:

<input {...input.multiple('input-name')} />

// or maybe as it was suggested in issue #13 which I really like
<input {...input.selectMultiple('input-name')} />

Here's why:

With the current implementation it might not be clear that the second argument opt is an option argument. Especially when most of the other inputs accept a second as a value attribute.

For example:

<input {...input.radio('foo', 'foo-val') /> // 2nd arg is the input value

<input {...input.checkbox('bar') />
<input {...input.checkbox('bar', 'bar-val') /> // 2nd arg is the input value

<input {...input.select('baz') />
<input {...input.select('baz', 'multiple') /> // 2nd arg is NOT the input value!

Even from an implementation perspective, you see how in the code we're calling that argument ownValue, so It can be a little confusing.

Therefore, this might not be very intuitive or consistent with the rest of the methods.

I'm in favor of inputs.selectMultiple which I think could work very well with intellisense/autocomplete! Let's go with that.

<input {...input.select('foo') />
<input {...input.selectMultiple('foo')} />

This brings to my second point. Could you please add typings for newly added method? The typings can be found here in this file:

interface Inputs {
select(name: string): Omit<InputProps, 'type'>;

Lastly, can I ask you to add some tests before I merge this. It would be ideal if there's a full test coverage.

Thanks again for the PR! 🙌

README.md Outdated Show resolved Hide resolved
@wsmd wsmd changed the title FIX #13 Add support for <select multiple> Feb 14, 2019
@wsmd wsmd added the enhancement Improve existing functionality label Feb 14, 2019
@gregleveque
Copy link
Contributor Author

I'm currently struggling for testing multiple select, because it's a CTRL + click event and the target value is only the last selected option.

The current test check if it handle an array of values but I can't add a new value because I can't find a way to implement this event.

Copy link
Owner

@wsmd wsmd left a comment

Choose a reason for hiding this comment

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

Nice job adding those tests!

I left a few comments about testing selecting multiple options and deselecting options. There's also a small linter error about an unused variable.

test/test-utils.js Outdated Show resolved Hide resolved
test/test-utils.js Outdated Show resolved Hide resolved
test/useFormState.test.js Outdated Show resolved Hide resolved
test/useFormState.test.js Show resolved Hide resolved
Copy link
Owner

@wsmd wsmd left a comment

Choose a reason for hiding this comment

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

This looks good! Thanks! 🙌

@wsmd wsmd merged commit 2b31fff into wsmd:master Feb 21, 2019
@gregleveque
Copy link
Contributor Author

Thank you for this awsome Hook 👍

gcampes pushed a commit to gcampes/react-use-form-state that referenced this pull request Mar 18, 2019
* FIX wsmd#13

* Refactoring to use 'selectMultiple'

* Unit test

* UPDATE Unit test for selectMultiple
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants