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 `keepInputCase` prop - Closes #1106 #1329

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@kauffecup

kauffecup commented Oct 26, 2016

Closes #1106

Add keepInputCase prop that leaves the casing of what the user types in the input, but still allows the control to ignore the case when filtering the results.

For example, with keepInputCase=true and ignoreCase=true, we get the following:

image

but still get:

image

The one caveat is it requires the async method to be case insensitive. But this way it doesn't change the input to be lowercased which, when using with a creatable, can prevent the user from typing it in correctly.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 26, 2016

Coverage Status

Coverage increased (+0.01%) to 92.277% when pulling 4abcff7 on kauffecup:keep-input-case into a8f4118 on JedWatson:master.

coveralls commented Oct 26, 2016

Coverage Status

Coverage increased (+0.01%) to 92.277% when pulling 4abcff7 on kauffecup:keep-input-case into a8f4118 on JedWatson:master.

@TheSharpieOne

This comment has been minimized.

Show comment
Hide comment
@TheSharpieOne

TheSharpieOne Oct 31, 2016

Contributor

This extra prop is really not need. There is an error in the logic which should be fixed. Currently, the _onInputChange method modifies the inputValue which it passes to both the onChange prop and the loadOptions method, returning the result of the loadOptions method, which is just the manipulated value that was passed it to (when no cache is found).

Really, IMHO, _onInputChange should pass the original inputValue to the user-defined onChange prop and return it (or nothing) to the calling function.
If user-defined onChange prop is not there or returns nothing, it will leave the input as-is (as the end-user typed). If it does return something, use the result of the user-defined onChange prop, allowing the user (in user-land) to manipulate the inputValue which is eventually displayed back to the user.

Either way, ignoreCase affecting the inputValue displayed back to the user is not a feature which should be preserved, but a defect which should be eliminated. After all, ignoreCase is supposed to be for searching, nothing more.

Contributor

TheSharpieOne commented Oct 31, 2016

This extra prop is really not need. There is an error in the logic which should be fixed. Currently, the _onInputChange method modifies the inputValue which it passes to both the onChange prop and the loadOptions method, returning the result of the loadOptions method, which is just the manipulated value that was passed it to (when no cache is found).

Really, IMHO, _onInputChange should pass the original inputValue to the user-defined onChange prop and return it (or nothing) to the calling function.
If user-defined onChange prop is not there or returns nothing, it will leave the input as-is (as the end-user typed). If it does return something, use the result of the user-defined onChange prop, allowing the user (in user-land) to manipulate the inputValue which is eventually displayed back to the user.

Either way, ignoreCase affecting the inputValue displayed back to the user is not a feature which should be preserved, but a defect which should be eliminated. After all, ignoreCase is supposed to be for searching, nothing more.

Don't normalize the input text on creatable
This prevents entered text from being case-folded / having its
diacritics stripped and thus being mangled, by splitting the inputValue
(user's entered value) and normalizedInputValue (the value used for
loadOptions).

Similar to the fix in #1329, except that the behavior is always on (not
controlled by a prop), and the loadOptions behavior is unchanged -- the
loadOptions fn always gets the normalized value, as before.

Fixes #1106.
@kauffecup

This comment has been minimized.

Show comment
Hide comment
@kauffecup

kauffecup Nov 11, 2016

@TheSharpieOne, is @bardediger's commit what you're looking for? if so i can cherry pick it onto my branch or close this PR and he can open a new one?

kauffecup commented Nov 11, 2016

@TheSharpieOne, is @bardediger's commit what you're looking for? if so i can cherry pick it onto my branch or close this PR and he can open a new one?

@TheSharpieOne

This comment has been minimized.

Show comment
Hide comment
@TheSharpieOne

TheSharpieOne Nov 11, 2016

Contributor

His commit looks good to me

Contributor

TheSharpieOne commented Nov 11, 2016

His commit looks good to me

@kauffecup

This comment has been minimized.

Show comment
Hide comment
@kauffecup

kauffecup Nov 11, 2016

ok! this PR now contains only his commit!

kauffecup commented Nov 11, 2016

ok! this PR now contains only his commit!

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 11, 2016

Coverage Status

Coverage increased (+0.4%) to 92.526% when pulling 53af1bd on kauffecup:keep-input-case into ad337ee on JedWatson:master.

coveralls commented Nov 11, 2016

Coverage Status

Coverage increased (+0.4%) to 92.526% when pulling 53af1bd on kauffecup:keep-input-case into ad337ee on JedWatson:master.

@caguthrie

This comment has been minimized.

Show comment
Hide comment
@caguthrie

caguthrie Dec 16, 2016

Can we please merge this PR? Happy to help if there is anything we are waiting on here but I don't think there is

caguthrie commented Dec 16, 2016

Can we please merge this PR? Happy to help if there is anything we are waiting on here but I don't think there is

sumeetgyani added a commit to smashgg/react-select that referenced this pull request Jan 10, 2017

Fix uppercase bug, see description
This prevents entered text from being case-folded / having its
diacritics stripped and thus being mangled, by splitting the inputValue
(user's entered value) and normalizedInputValue (the value used for
loadOptions).

Similar to the fix in #1329, except that the behavior is always on (not
controlled by a prop), and the loadOptions behavior is unchanged -- the
loadOptions fn always gets the normalized value, as before.

JedWatson@53af1bd
@BossGrand

This comment has been minimized.

Show comment
Hide comment
@BossGrand

BossGrand Mar 14, 2017

Whats the status on this?

BossGrand commented Mar 14, 2017

Whats the status on this?

@jperl

This comment has been minimized.

Show comment
Hide comment
@jperl

jperl Mar 15, 2017

We are running into this issue as well. Is there any way we can help get this merged?

jperl commented Mar 15, 2017

We are running into this issue as well. Is there any way we can help get this merged?

@davidemerritt

This comment has been minimized.

Show comment
Hide comment
@davidemerritt

davidemerritt May 10, 2017

I would love to see this merged in as well.

davidemerritt commented May 10, 2017

I would love to see this merged in as well.

@agirton

This comment has been minimized.

Show comment
Hide comment
@agirton

agirton May 24, 2017

Collaborator

I'm in agreement with @TheSharpieOne, the _onInputChange method should not be mutating the input, this should be done by the filter. I don't think this is the right way to go. I will open an issue and then someone can grab 😄

Edit Just realized you aren't creating a prop any longer. What's the benefit of adding the additional argument over just removing those mutations? I understand it will make the method case insensitive, but this can be handle in user land if needed.

Collaborator

agirton commented May 24, 2017

I'm in agreement with @TheSharpieOne, the _onInputChange method should not be mutating the input, this should be done by the filter. I don't think this is the right way to go. I will open an issue and then someone can grab 😄

Edit Just realized you aren't creating a prop any longer. What's the benefit of adding the additional argument over just removing those mutations? I understand it will make the method case insensitive, but this can be handle in user land if needed.

@zwright

This comment has been minimized.

Show comment
Hide comment
@zwright

zwright Jun 6, 2017

Here's another vote for getting this merged.

zwright commented Jun 6, 2017

Here's another vote for getting this merged.

@sponrad

This comment has been minimized.

Show comment
Hide comment
@sponrad

sponrad Jun 14, 2017

the _onInputChange method should not be mutating the input

@agirton Agree!

We are also having this issue. Could be as simple as this change: sponrad@029e56d

sponrad commented Jun 14, 2017

the _onInputChange method should not be mutating the input

@agirton Agree!

We are also having this issue. Could be as simple as this change: sponrad@029e56d

@gwyneplaine

This comment has been minimized.

Show comment
Hide comment
@gwyneplaine

gwyneplaine Oct 24, 2017

Collaborator

@kauffecup thanks for this, a separate body of work has resolved the root issue behind this PR in the manner that @agirton suggested. You can see it here #1858

Collaborator

gwyneplaine commented Oct 24, 2017

@kauffecup thanks for this, a separate body of work has resolved the root issue behind this PR in the manner that @agirton suggested. You can see it here #1858

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment