Skip to content

Conversation

@alexeybondarenko
Copy link
Contributor

@alexeybondarenko alexeybondarenko commented Nov 2, 2016

#74

package.json Outdated
"react": "0.14.x || 15.x.x"
},
"devDependencies": {
"babel-eslint": "^7.1.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this in this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because Eslint is configured to use babel-eslint as parser and many IDE (such as Atom) require this package to be installed locally.

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 can move this change to separate PR

src/index.js Outdated
function getSelection (el) {
var start, end, rangeEl, clone;

if( el.selectionStart !== undefined){
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: if (typeof el.selectionStart !== 'undefined') { (spacing should follow the rest of the codebase)

src/index.js Outdated
var rangeEl;

try {
if( el.selectionStart !== undefined){
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit spacing

src/index.js Outdated
rangeEl = el.createTextRange()
clone = rangeEl.duplicate()

rangeEl.moveToBookmark(document.selection.createRange().getBookmark());
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: no semicolons

@iamdustan
Copy link
Collaborator

some nits to get the codestyle in line. the change looks okay to me, though I’d like @jquense to approve, too. He knows the DOM specifics of inputs far better than I!

@jquense
Copy link
Collaborator

jquense commented Nov 2, 2016

this code has worked fine for me for a long time now. The one caveat is that it aggressively focuses the input (otherwise it'll throw in old IE), before fetching or setting the selection. It shouldn't be a problem but i'd run and confirm it's not changing accidentally conflicting with any other code in the input trying to focus.

@alexeybondarenko
Copy link
Contributor Author

I've fixed code style.

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