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

Fixing indexing and text offset problems in injectCFIMarkerIntoText. #1

Closed
wants to merge 1 commit into from

Conversation

marcuswu
Copy link

This change fixes a couple of situations where injectCFIMarkerIntoText would index into the text node list or calculate offsets into text incorrectly. It also makes the method more CFI spec compliant by allowing for injection after the last character of a text node.

…also making it more spec compliant by allowing for injection after the last character of a text node (after the text node itself)
@bormind
Copy link
Contributor

bormind commented Aug 19, 2013

Hi Marcus, thank you for looking into this.

Currently cfi code in readium-shared-js/libemub_cfi.js is not automatically synchronized with epubcfi project (https://github.com/readium/EPUBCFI) but this is where emub_cfi.js comes from. We just replace the one in readium-shared-js/lib with one from EPUBCFI project.
Could you please make sure that you changes are in EPUBCFI project. Than we will just do the replace of the emub_cfi.js in readium-shared-js/lib/emub_cfi.js. Otherwise we may loose your changes.

Boris.

@marcuswu
Copy link
Author

Ok, thanks. I've submitted a pull request to the EPUBCFI project.

@marcuswu
Copy link
Author

The pull request to the EPUBCFI project has been accepted. Should I just close this one?

@bormind
Copy link
Contributor

bormind commented Aug 20, 2013

Marcus, updated epub_cfi now in the readium-shared-js. You can close the request.

Thanks.

@marcuswu
Copy link
Author

Thanks

@marcuswu marcuswu closed this Aug 20, 2013
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.

2 participants