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

Adds support for copy/paste #25

Merged
merged 17 commits into from
Dec 12, 2017
Merged

Adds support for copy/paste #25

merged 17 commits into from
Dec 12, 2017

Conversation

lukaszlenart
Copy link
Contributor

User can paste a name of a suggestion she's looking for

@arigoldx arigoldx self-assigned this Dec 8, 2017
@arigoldx arigoldx self-requested a review December 8, 2017 14:37
@arigoldx
Copy link
Member

arigoldx commented Dec 8, 2017

In terms of testing, unit tests pass & I'll give this a whirl.

'coupla lil questions for curiosity's sakes:

What happened that we need to add the left and right arrows to the non printable keys?

aaand,

What was the trick with all that version-number craziness?

(thanks ahead of time for the lesson :))

@lukaszlenart
Copy link
Contributor Author

re: left & right

each time when you were navigating throughout the text (going left or right), a request was sent back to the server to fetch possible suggestions, I assumed that this wasn't an expected behaviour.

re: version carziness

just to have a green build on Travis 😆

@arigoldx
Copy link
Member

arigoldx commented Dec 11, 2017

The complied JavaScript worked like a charm so I'm all sorts of 👍

@pdyraga for GK?

@arigoldx
Copy link
Member

Noooooow I get it.. 'just hit me that the way to check the Travis build is to just keep committing until its green 😆

@@ -6,18 +6,22 @@ elemicaSuggest 0.9.2-SNAPSHOT
*/

(function() {
var indexOf = [].indexOf || function(item) { for (var i = 0, l = this.length; i < l; i++) { if (i in this && this[i] === item) return i; } return -1; };
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to define indexOf here? Starting from IE9 it should be available in Array: https://www.w3schools.com/jsref/jsref_indexof_array.asp

Is it for IE8 compatibility? I think we can drop a support for this version in the elemica-suggest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, it's what npm run-script dist produced. I didn't change any option related to IEs

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting... Let's trust coffee's compiler, we have no better option 😉

@@ -27,6 +31,7 @@ elemicaSuggest 0.9.2-SNAPSHOT
ALT = 18;
OS_LEFT = 91;
OS_RIGHT = 92;
NON_PRINTALBE_KEYS = [SHIFT, CTRL, ALT, OS_LEFT, OS_RIGHT, TAB, LEFT_ARROW, RIGHT_ARROW];
Copy link
Contributor

Choose a reason for hiding this comment

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

PRINTALBE -> PRINTABLE ? 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you can blame natives, I have just added LEFT_ARROW and RIGHT_ARROW ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@lukaszlenart
Copy link
Contributor Author

Re-tested this new dist with mercury and everything looks good.

@pdyraga
Copy link
Contributor

pdyraga commented Dec 12, 2017

👍

@pdyraga pdyraga merged commit 43eff65 into master Dec 12, 2017
@pdyraga pdyraga deleted the copy-paste branch December 12, 2017 10:56
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.

3 participants