fixes issue #84, for pasting with delimiters #90

ajmas · 2017-08-14T18:17:35Z

Changes to support pasting with delimiters, but currently missing a test case, since I am not sure how to write one that support the browser clipboard. I am a little new to Istanbul code coverage. Any suggestions would be appreciated.

coveralls · 2017-08-14T18:23:27Z

Coverage decreased (-9.8%) to 89.56% when pulling d364403 on ajmas:issue-84-pasting-and-delimeters into 837e480 on i-like-robots:master.

coveralls · 2017-08-14T21:20:12Z

Coverage decreased (-1.6%) to 97.802% when pulling 5fae01c on ajmas:issue-84-pasting-and-delimeters into 837e480 on i-like-robots:master.

i-like-robots · 2017-08-14T21:24:17Z

lib/ReactTags.js

+
+    if (data && delimiterChars.length > 0) {
+      // split the string based on the delimiterChars as a regex
+      const tags = data.split(new RegExp(delimiterChars.join('|'), 'g'))


I think this can be simplified a little with a group:

data.split(new RegExp('[' + delimiterChars.join('') + ']'))

I'll make the change

Remember to check for active characters though: delimiterChars may contain chars that completely change the semantics of the group (such as ^ at the start, or - anywhere in the group pattern).

Doing some tests, the original data.split(new RegExp(delimiterChars.join('|'), 'g')) seems more robust than data.split(new RegExp('[' + delimiterChars.join('') + ']')).

Results:

delimiterChars = ['^',',',';'] data = 'green; blue, white' data.split(new RegExp(delimiterChars.join('|'), 'g')) // result: ["green", " blue", " white"] data.split(new RegExp('[' + delimiterChars.join('') + ']')) // result: ["", "", "", "", "", ";", "", "", "", "", ",", "", "", "", "", "", ""]

I am tempted to leave the original code based on this test.

note that the original code still needs checks for active characters, too: if someone uses . to act as tag separator, then suddenly things will go horribly wrong without escaping. To do this safely, you'd have to not use regexp at all, but instead set some restrictions on delimiterChar so that you can safely tokenize with its content:

function splitOnDelimiters(input, delimiterChars) { const parts = []; const chars = Array.from(input); for(let i=chars.length-1; i>=0; i--) { if (delimiterChars.indexOf(chars[i]) > -1) { parts.push( chars.splice(i+1, chars.length - i).join('') ); chars.pop(); } } if (chars.length > 0) { parts.push(chars.join('')); } return parts.filter(v => v).reverse() }

(and then of course this code would work because delimiterChars was set to only allow individual characters as delimiters, not "two or more" per delimiting entry)

If things have to stay regexp, that's an option. Though delimiterChars should probably be made sure to contain only single characters, with a warning or error when improper data is passed like delimiterChars: ['^', '.*\d+\s','lol'].

Good point. I didn't realise the can of worms I was opening here.
My expectation is that the delimiters are single characters, though I am not sure if there are use cases that require a sequence? @i-like-robots any thoughts on this?

we could probably enforce them in the constructor, with warning when they are an improper format.

There's an escapeForRegExp function within the lib somewhere that could be pulled in to deal with escaping any special chars. I'm always of the opinion libraries like this should be relatively dumb but easily configurable so I'm not keen on enforcing strict rules like this.

Okay, I'll do this. Just one thing, is that we may find this causes inconsistent behaviour WRT to handleKeyDown, since the logic there won't deal with multi-character delimiters. I'll compromise and add a note in the readme.

i-like-robots · 2017-08-14T21:27:06Z

lib/ReactTags.js

+        // due to the context of the operation
+        if (tags[i].length > 0) {
+          // look to see if the tag is already known
+          const matchIdx = this.suggestions.state.options.findIndex((suggestion) => {


Rather than reaching into another component the suggestions are referenced at this.props.suggestions

I has been using the same logic as handleKeyDown, so we should probably have that updated too eventually, though tempted not to touch handleKeyDown in this change set?

Good spot, yes please keep this PR focussed and I'll move in with my sweeping brush after

i-like-robots · 2017-08-14T21:29:52Z

lib/ReactTags.js

+  handlePaste (e) {
+    // allow over-ride, if there is a need
+    if (this.props.onPaste) {
+      return this.props.onPaste(e)


For consistency I think this option should be named handlePaste

the general React pattern is to call handlers onSomething though? (as this is the keyword that gets passed in through JSX's <ReactTags onPaste={...} />

That had been by understanding too. Maybe the way forward would be to go for consistency as suggested and maybe replace and deprecate as a block in a future version?

Done, but have opened issue #91

Pomax · 2017-08-14T21:52:22Z

lib/ReactTags.js

+    if (data && delimiterChars.length > 0) {
+      // split the string based on the delimiterChars as a regex
+      const tags = data.split(new RegExp(delimiterChars.join('|'), 'g'))
+      for (let i = 0; i < tags.length; i++) {


depending on codestyle: since i is only used for accessors, tags.forEach( tag => { ... }) would not look out of place here

Pomax · 2017-08-14T21:54:55Z

spec/ReactTags.spec.js

@@ -371,6 +375,17 @@ describe('React Tags', () => {

      expect($$('.custom-tag').length).toEqual(2)
    })
+
+    it('can receive tags through paste, respecting delimiters', () => {


with the regexp update, you'll definitely want to put up some tests against active chars, like delimiterChars = [']','.*[']. It's not too hard to hijack someone's React application with a user script, so making sure the code doesn't die when an intentionally bad regex is dropped in will be worth the effort.

Added both validation in constructor and escape chars. Also updated test case.

coveralls · 2017-08-14T22:55:10Z

Coverage increased (+0.06%) to 99.451% when pulling 89b68b3 on ajmas:issue-84-pasting-and-delimeters into 837e480 on i-like-robots:master.

Pomax · 2017-08-14T22:55:18Z

lib/ReactTags.js

@@ -56,6 +56,43 @@ class ReactTags extends React.Component {
    this.setState({ query })
  }

+  handlePaste (e) {
+    // allow over-ride, if there is a need
+    if (this.props.handlePaste) {


I'd still advocate sticking with React conventions and calling this onPaste, but take that as just an opinion of a fellow react component dev rather than anything more than that.

See issue #91

ajmas · 2017-08-14T23:02:49Z

@Pomax for the user specified event naming conventions, I have opened issue #91

ajmas

I have added a check in the constructor. It will throw an error if any entry is greater than one character in length

ajmas · 2017-08-14T22:36:13Z

lib/ReactTags.js

+    // get the text data from the clipboard
+    const data = e.clipboardData.getData('Text')
+
+    if (data && delimiterChars.length > 0) {


What is the expectation if delimiterChars.length is 0? Currently nothing is converted to tag. I am want to see if this is reasonable or whether we should treat it as one block?

If there are no delimiterChars, then ReactTags can't do its job, I think? It needs a list to be able to chunk up input in the first place so if it does not have that list it can't convert string data into tags. Some options are to issue a warning ("no delimiter characters specified for onPaste handling by ReactTags") or to simply break out of the handler function before any code actually gets run.

@i-like-robots is the idea that delimiterChars and delimiters are mutually exclusive (e.g. if you need real letters, don't use numbers, and vice versa) or are they complementary? (in which case I might try to build a map that can tell which key is which keyboard event code, because the difference between those two is still ridiculous =D)

Would this be okay:

else if (!delimiterChars || delimiterChars.length === 0) { if (console) { console.warn('no delimiterChars specified, so ignoring paste operation') } }

For the evolution of delimiterChars and delimiters, could that be another ticket, given there is probably a lot of subtle use cases? For example the enter key can have two different values, depending on whether it is on the alphanumeric or num-pad section of the keyboard. I don't have a num-pad to test with.

Should that unification work be done as part of issue #81?

coveralls · 2017-08-14T23:19:07Z

Coverage increased (+0.08%) to 99.468% when pulling 88a33b2 on ajmas:issue-84-pasting-and-delimeters into 837e480 on i-like-robots:master.

i-like-robots · 2017-08-15T07:29:06Z

Thank you again @ajmas - great testing too!

Changes include - Not preventing multi-character delimiters - Indicating multi-chracter delimiters are only supported in paste operations, mainly because handleKeyDown will not deal with them - Changing the default delimiterChars to be consistent with the set of default delimiter keycodes

coveralls · 2017-08-15T16:12:44Z

Coverage increased (+0.07%) to 99.457% when pulling 56b2d10 on ajmas:issue-84-pasting-and-delimeters into 837e480 on i-like-robots:master.

ajmas · 2017-09-07T23:24:27Z

What do you think is missing before the PR can be accepted? In the future we can probably remove delimiters in favour of delimiterChars, though that will probably require a major version change?

ajmas · 2017-09-08T04:03:38Z

Note I have discovered a small issue with Chrome 60 on Android 7, which doesn't seem to have implement the KeyboardEvent.key attribute. I'll need to see if 'keyboardevent-key-polyfill' package could address this limitation.

- Allow example to be access from seconds device, such as allowing to be tested from a mobile - Added 'demiterChars' to example

coveralls · 2017-09-08T04:30:45Z

Coverage increased (+0.08%) to 99.465% when pulling 4a6962c on ajmas:issue-84-pasting-and-delimeters into 837e480 on i-like-robots:master.

coveralls · 2017-09-08T04:33:22Z

Coverage increased (+0.08%) to 99.465% when pulling 52cad74 on ajmas:issue-84-pasting-and-delimeters into 837e480 on i-like-robots:master.

i-like-robots · 2017-09-08T14:22:17Z

@ajmas Hey sorry, I've let this slide... a combination of being super busy and also dealing with illness.

I'm very surprised to hear about the KeyboardEvent.key issue... I think it can be worked around but irritating nonetheless.

Will pick this up very soon.

coveralls · 2017-09-08T14:24:19Z

Coverage increased (+0.08%) to 99.465% when pulling 52cad74 on ajmas:issue-84-pasting-and-delimeters into 837e480 on i-like-robots:master.

ajmas · 2017-09-09T04:36:47Z

@i-like-robots no issues. I just wanted to be sure it wasn't something my side blocking things. Take of yourself and let me know when you are ready.

ajmas · 2017-09-25T20:30:01Z

I am working on a set of changes that leverages onChange, which may mean the onPaste approach is no longer necessary. I’ll make the PR when I get the chance and then we can discuss.

Pomax · 2018-01-18T18:46:58Z

given the merge conflict, is this PR still slated for review and landing, or has the codebase passed it by already?

i-like-robots · 2018-01-19T08:19:26Z

HI @Pomax - I've been leaving it open for reference. I still intend on dealing with delimiters pasted into the input in V6 (#106). I had a chat with one of the UX-ers at work before Christmas and we agreed that it would only make sense when allowNew was true.

I keep letting this slide... my to-do list is always so long! I'll try my best to get a V6 beta out this weekend.

Pomax · 2018-05-31T17:21:34Z

This keeps showing up in my work queues as an open PR that has me mentioned, is it possible to close this? You can link to it in an issue about this work and even when closed always be able to find it, but that would at least take it out of my github queues =)

ajmas added 3 commits August 14, 2017 14:09

fixes issue i-like-robots#84, for pasting with delimiters

6ced8e0

removed trailing whitespace

27939c1

fixed issues causing linter to complain

d364403

Added test case for onPaste

5fae01c

i-like-robots reviewed Aug 14, 2017

View reviewed changes

Pomax reviewed Aug 14, 2017

View reviewed changes

Changes following review

89b68b3

Pomax reviewed Aug 14, 2017

View reviewed changes

ajmas commented Aug 14, 2017

View reviewed changes

Added missing changes, added extra tests

88a33b2

i-like-robots added this to the 5.5.0 milestone Aug 15, 2017

Added artifacts from prepublish

274fc81

Allow to access example from second device

4a6962c

- Allow example to be access from seconds device, such as allowing to be tested from a mobile - Added 'demiterChars' to example

Removed 'allowNew' attr from example

52cad74

i-like-robots closed this Sep 8, 2017

i-like-robots reopened this Sep 8, 2017

i-like-robots modified the milestones: 5.5.0, 6.0.0 Nov 15, 2017

i-like-robots closed this Jun 2, 2018

fixes issue #84, for pasting with delimiters #90

fixes issue #84, for pasting with delimiters #90

Conversation

ajmas commented Aug 14, 2017

coveralls commented Aug 14, 2017 • edited Loading

coveralls commented Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pomax Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pomax Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajmas Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajmas Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pomax Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

ajmas Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajmas Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajmas commented Aug 14, 2017

ajmas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajmas Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Aug 14, 2017 • edited Loading

i-like-robots commented Aug 15, 2017

coveralls commented Aug 15, 2017 • edited Loading

ajmas commented Sep 7, 2017 • edited Loading

ajmas commented Sep 8, 2017

coveralls commented Sep 8, 2017 • edited Loading

coveralls commented Sep 8, 2017 • edited Loading

i-like-robots commented Sep 8, 2017

coveralls commented Sep 8, 2017 • edited Loading

ajmas commented Sep 9, 2017

ajmas commented Sep 25, 2017

Pomax commented Jan 18, 2018

i-like-robots commented Jan 19, 2018 • edited Loading

Pomax commented May 31, 2018

coveralls commented Aug 14, 2017 •

edited

Loading

coveralls commented Aug 14, 2017 •

edited

Loading

Pomax Aug 14, 2017 •

edited

Loading

Pomax Aug 14, 2017 •

edited

Loading

ajmas Aug 14, 2017 •

edited

Loading

ajmas Aug 14, 2017 •

edited

Loading

Pomax Aug 14, 2017 •

edited

Loading

ajmas Aug 14, 2017 •

edited

Loading

ajmas Aug 14, 2017 •

edited

Loading

coveralls commented Aug 14, 2017 •

edited

Loading

ajmas Aug 15, 2017 •

edited

Loading

coveralls commented Aug 14, 2017 •

edited

Loading

coveralls commented Aug 15, 2017 •

edited

Loading

ajmas commented Sep 7, 2017 •

edited

Loading

coveralls commented Sep 8, 2017 •

edited

Loading

coveralls commented Sep 8, 2017 •

edited

Loading

coveralls commented Sep 8, 2017 •

edited

Loading

i-like-robots commented Jan 19, 2018 •

edited

Loading