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

[Labs] Only unique values in TagInput #1277

Closed
jacekjagiello opened this issue Jun 25, 2017 · 6 comments
Closed

[Labs] Only unique values in TagInput #1277

jacekjagiello opened this issue Jun 25, 2017 · 6 comments

Comments

@jacekjagiello
Copy link
Contributor

jacekjagiello commented Jun 25, 2017

Possibility to have only unique values in TagInput component.

Concerns:

  • Should we support this out of the box? Maybe it's responsible of user code which controls values passed to TagInput component?
  • Should we call some kind of method from props, when I'm trying to add value that already exists?
  • Should we define uniqueness only base on value?
@giladgray
Copy link
Contributor

giladgray commented Jun 25, 2017

@jacekjagiello I do not think this feature belongs in TagInput--it goes against the fully controlled nature of the API. Also, the code is simple enough (in your PR) that there's little value in including it directly in the component: it's just a single filter, and I'm sure that defining uniqueness based only on strict string equality will not be sufficient across the board.

Something that I thought about while developing this initial version was supporting a return value from onAdd to control the input-clearing behavior: return false to say "I don't want to add this," and the component will ignore the event and not clear the input. That alone would allow users to implement whatever sort of behavior they like. (Same thing for onRemove is less valuable cuz there's no behavior in the component--removing the item is user code responsibility.)

How does that sound, instead of this feature?

@jacekjagiello
Copy link
Contributor Author

jacekjagiello commented Jun 26, 2017

@giladgray Yeah, valid points. Supporting a return value definitely sound better. ;) Can you elaborate this feature more? I mean, for me it means something as simple as:

const onAddResult = Utils.safeInvoke(this.props.onAdd, value);
if (onAddResult !== false) {
    this.setState({ inputValue: "" });
}

When onAdd can return boolean | void. Is that all or I miss something?

Ps. Sorry for wrong issue number in commits. I referred this issue instead of 1220, but I've fixed commit names.

@llorca
Copy link
Contributor

llorca commented Jun 26, 2017

@jacekjagiello I agree with Gilad, let's keep the component fully controlled. Your little snippet for onAdd?: (value: string) => boolean | void seems good, would you like to try and open a PR for it? You could then add a "Unique values only" toggle to the example to showcase this upgraded onAdd callback 💯

@jacekjagiello
Copy link
Contributor Author

@llorca Sure, can you close this issue and start a new one? Or do you want to keep this under current issue?

@llorca
Copy link
Contributor

llorca commented Jun 27, 2017

@jacekjagiello We can keep this issue open and close up the current PR 👍

@cmslewis
Copy link
Contributor

@llorca @giladgray @jacekjagiello - I took a stab at this in #1309. Lemme know how that looks.

@cmslewis cmslewis self-assigned this Jun 30, 2017
@cmslewis cmslewis added this to the 1.22.0 milestone Jun 30, 2017
@llorca llorca modified the milestones: 1.22.0, 1.23.0 Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants