If selectedItem is controlled, 'inputValue' is not updated when an it… #252

yp · 2017-11-07T10:39:13Z

…em is selected but only when prop 'selectedItem' is updated.

What: see issue #243

Why: see issue #243

How: inputValue is set to '' if selectedItem is a controlled prop when an item is selected.

Checklist:

Documentation N/A
Tests
Ready to be merged
Added myself to contributors table

…em is selected but only when prop 'selectedItem' is updated.

codecov-io · 2017-11-07T10:43:26Z

Codecov Report

Merging #252 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #252   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         296    299    +3     
  Branches       71     72    +1     
=====================================
+ Hits          296    299    +3

Impacted Files	Coverage Δ
src/downshift.js	`100% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22ed3d7...f69b1e3. Read the comment docs.

… not the inputValue).

kentcdodds

Ok, I think that I see where this is going. In your example, it does appear to be updating the inputValue to the selected item anyway. But the example doesn't demonstrate the behavior you want (setting the selectedItem to ''). Could you update the example so we can be sure what the experience looks like and that it's what you want? You could simply add a button to set it to an empty string or something.

Thanks so much for doing this!

kentcdodds · 2017-11-07T16:46:19Z

src/__tests__/downshift.props.js

  const changes = {
    type: Downshift.stateChangeTypes.unknown,
    selectedItem: itemToSelect,
-    inputValue: itemToSelect,


This requires that we publish a breaking change. I'm not averse to breaking changes, but I do like to avoid them (and do them in bulk) if I can... 🤔

kentcdodds · 2017-11-07T16:46:44Z

src/downshift.js

        selectedItem: item,
-        inputValue: this.props.itemToString(item),
+        inputValue: this.isControlledProp('selectedItem')
+          ? ''


Maybe we should reset to this.props.defaultInputValue?

It is indeed a good suggestion.

… selectedItem is a controlled prop.

…ponents to select different items from a list.

kentcdodds

Ok, I have an idea. I want to avoid making a breaking change right now and I'd rather hold off until we have other breaking changes ready to go. So here's what a propose:

A new prop (Maybe called breakingChanges) that's an object which you can pass flags for behavior to opt-into things that will be breaking changes in the next release.

This way folks can enable these things and when the next major version happens, then they can simply remove the prop and everything will continue to work as before for them.

So it could be:

<Downshift breakingChanges={{resetInputOnSelection: true}} />

Then in the code we'd reference this to know whether to continue with the current behavior or use the new behavior. We'd be able to document this opt-in support in one place and when it comes time to push a major version we can go throughout the codebase and remove all of these. I think it should be straightforward. What do you think?

arahansen · 2017-11-08T16:43:29Z

@kentcdodds Would you prefer a prop compared to a next branch or a 2.0-alpha type release?

kentcdodds · 2017-11-08T17:10:49Z

I only use those when a breaking change is imminent. But I can't think of other things that justify a major release at this time, so I think this is a good compromise. I want to avoid API churn.

yp · 2017-11-08T17:17:25Z

What about introducing a new prop (say resetInputValue) that indicates what to do when resetting? For example null (default) forces the use of itemToString while if it is a string it is used as the new inputValue.

kentcdodds · 2017-11-08T17:26:04Z

I'd considered that as well, but unless I'm mistaken, the current behavior would actually never be preferred, so I'd like to eventually get rid of it. Every prop we add comes with more complexity for the codebase and the API. So I'd rather go with a temporary prop if the behavior possible.

But I could be wrong. Is there a common enough situation where someone wants the reset behavior to be something else? I'd rather defer people to controlling inputValue if they can...

yp · 2017-11-08T17:29:55Z

This is a sensible position. I am fine with either solution (opt-in or next/alpha branch).

kentcdodds · 2017-11-08T18:31:30Z

Let's go with the opt-in. Thank you so much!

kentcdodds

Cool! This is looking good. 2 Things:

Let's document this somewhere in the README. Maybe it's own section? Wherever makes sense.
Could you make the examples use the new prop so we see the behavior?

Thanks! I'm feeling pretty good about this 👍

kentcdodds · 2017-11-09T16:17:52Z

src/downshift.js

-        inputValue: this.props.itemToString(item),
+        inputValue:
+          this.isControlledProp('selectedItem') &&
+          this.props.breakingChanges &&


Doing the check for this.props.breakingChanges is unnecessary because the default prop is an empty object 👍

The idea was to be on the safe side in case someone erroneously specifies null. But it is up to you.

Good point. I think that most people wont be using this prop and of those, not many will make it dynamic. I expect that folks will inline an object who want to use this prop, so let's just remove the check 👍 Thanks!

yp · 2017-11-09T16:33:05Z

Sorry, I forgot to comment the new commits.

Could you please write yourself the new prop? I am not confident in being able to do a good job. (I think you should be able to contribute to this PR.)
I forgot the examples. I'll do something tomorrow.
Thanks for your work.

yp · 2017-11-09T16:36:11Z

BTW, I prefixed the prop name with v2 in order to be clear about the version in which the breaking change will appear. I can revert to the unprefixed version if you wish so.

kentcdodds · 2017-11-09T16:47:51Z

BTW, I prefixed the prop name with v2 in order to be clear about the version in which the breaking change will appear. I can revert to the unprefixed version if you wish so.

I don't mind it, but I do feel it's a little redundant. All properties in breakingChanges will be broken in v2. I don't expect anything to be v3 in that object.

Could you please write yourself the new prop? I am not confident in being able to do a good job. (I think you should be able to contribute to this PR.)

Sure, I can do that 👍

… prop

kentcdodds · 2017-11-12T06:11:36Z

Thank you so much @yp! 🎉

If selectedItem is controlled, 'inputValue' is not updated when an it…

b20df06

…em is selected but only when prop 'selectedItem' is updated.

Add example for the use case that controls only the selectedItem (and…

0ad4edb

… not the inputValue).

kentcdodds reviewed Nov 7, 2017

View reviewed changes

yp added 2 commits November 8, 2017 09:58

Reset to defaultInputValue instead of '' when an item is selected and…

ed49d20

… selectedItem is a controlled prop.

Add example demonstrating the use of several downshift-controlled com…

41aa498

…ponents to select different items from a list.

kentcdodds reviewed Nov 8, 2017

View reviewed changes

Add the opt-in prop.

f319c82

kentcdodds reviewed Nov 9, 2017

View reviewed changes

kentcdodds mentioned this pull request Nov 9, 2017

Add the option suppressRefError to getRootProps. #241

Merged

4 tasks

yp and others added 4 commits November 10, 2017 10:21

remove the redundant prefix 'v2' from the flag of the breakingChanges…

be7e9f6

… prop

remove redundant check

3fd76e1

update the example

ebff460

Update README.md

f69b1e3

kentcdodds merged commit 831aeab into downshift-js:master Nov 12, 2017

If selectedItem is controlled, 'inputValue' is not updated when an it… #252

If selectedItem is controlled, 'inputValue' is not updated when an it… #252

Uh oh!

Conversation

yp commented Nov 7, 2017

Uh oh!

codecov-io commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

kentcdodds Nov 7, 2017

Choose a reason for hiding this comment

Uh oh!

kentcdodds Nov 7, 2017

Choose a reason for hiding this comment

Uh oh!

yp Nov 8, 2017

Choose a reason for hiding this comment

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

arahansen commented Nov 8, 2017

Uh oh!

kentcdodds commented Nov 8, 2017

Uh oh!

yp commented Nov 8, 2017

Uh oh!

kentcdodds commented Nov 8, 2017

Uh oh!

yp commented Nov 8, 2017

Uh oh!

kentcdodds commented Nov 8, 2017

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

kentcdodds Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

yp Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

kentcdodds Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

yp Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

yp commented Nov 9, 2017

Uh oh!

yp commented Nov 9, 2017

Uh oh!

kentcdodds commented Nov 9, 2017

Uh oh!

kentcdodds commented Nov 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Nov 7, 2017 •

edited

Loading