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

Fixes issue with calculation position from index with BR tags in ie #305

Merged
merged 2 commits into from Mar 20, 2015

Conversation

butsjoh
Copy link
Contributor

@butsjoh butsjoh commented Mar 12, 2015

When you focus on a line that only has a BR tag in it ie will set the start of the range always to 1 which is inconsistent with other browsers which return 0.

@@ -122,6 +122,9 @@ class Selection
return this._decodePosition(leaf.node, offset)

_positionToIndex: (node, offset) ->
if dom.isIE(10) and node.innerHTML == '<br>' and offset == 1
Copy link
Member

Choose a reason for hiding this comment

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

Can you collapse this into one line:

offset = 0 if dom.isIE(10) and node.tagName == 'BR' and offset == 1

@butsjoh
Copy link
Contributor Author

butsjoh commented Mar 18, 2015

@jhchen Done :)

@butsjoh
Copy link
Contributor Author

butsjoh commented Mar 18, 2015

@jhchen Need any help on extracting the modules out of the main codebase? I think quill should be split up into a core (all the dom, selection, delta stuff) and then each module can be added manually through using the core. I am happy to help on that if you provide the general guidelines you want to take it.

jhchen added a commit that referenced this pull request Mar 20, 2015
Fixes issue with calculation position from index with BR tags in ie
@jhchen jhchen merged commit 0030dfc into quilljs:develop Mar 20, 2015
@jhchen
Copy link
Member

jhchen commented Mar 20, 2015

Great thanks! Yes Quill ideally should be broken up into more independent parts but it's still unclear where the lines should be drawn and the inventory of dependencies.

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.

None yet

2 participants