Skip to content

Conversation

@lotrien
Copy link
Member

@lotrien lotrien commented Jan 12, 2018

To improve UX and let user intuitively add multiple tags, this commit
contains functionality that allows to add multiple tags by simply
clicking Enter button
Closes: #46

This component seems useless and I feel like we can go further without
it
@lotrien lotrien requested review from ikalnytskyi and malor January 12, 2018 22:25
To improve UX and let user intuitively add multiple tags, this commit
contains functionality that allows to add multiple tags by simply
clicking Enter button
Closes: #46
Copy link
Member

@malor malor left a comment

Choose a reason for hiding this comment

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

I like the UX! And you can use not only Enter, but also Space or Tab to separate tags.

Overall looks good to me 👍 Small nits inline.

syntax: '', // eslint-disable-line react/no-unused-state
};
this.onKeyPress = (e) => {
if (e.which === 13) { e.preventDefault(); }
Copy link
Member

Choose a reason for hiding this comment

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

Is there a constant for that? If not let's at least document, that 13 is a keycode of Enter

}

onTagAdded(tag) {
this.setState({ tags: [...this.state.tags, tag] });
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think we currently do not enforce it at the API level, but maybe we should prevent duplicates of tags on the same snippet? @ikalnytskyi what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense. Will you file an issue for API to enforce that?

Copy link
Member

Choose a reason for hiding this comment

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

@ikalnytskyi do you want to reject requests with duplicates with 400 Bad Request? Or just remove the duplicates before actually saving the snippet to the db?

Copy link
Member

Choose a reason for hiding this comment

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

@malor Honestly, I don't have strong opinion here. Both approaches have their own pros and cons. What do you think?

@ikalnytskyi ikalnytskyi merged commit c147379 into master Jan 13, 2018
@ikalnytskyi ikalnytskyi deleted the tag-input branch January 13, 2018 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants