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

fix to dom elements truncated with limit #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sam2x
Copy link

@sam2x sam2x commented Jul 2, 2016

Please review this patch, it fix a very annoying bug.

Bug Explanation: #6 (comment)

Summary:

  • It support plain text (ie: without html inside)
  • Afaik, the patch doesnt break any feautre of the directive (transparent to user)
  • Fix the bug where tags are truncated, which resulted to some content with html not parsed like " style='text-align:center'> Hey this is a long text that would not appear in less part!!!! </span>

Solution of this patch :

  • I use DOMParser recursively parse the content of vm.hmText (ie: data passed to hm-text directive)
  • If a text element is encountered, we take it in account to verify that the limit is not reached
  • If the limit is reached, the parsing is stopped, and the current element text is truncated. The function return the html parsed until the last tag which reached the limit.
  • Plain text are considered as a text element, so no edge case should appear.

@coveralls
Copy link

coveralls commented Jul 2, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 8542c3f on sam2x:fix_dom_elements_truncated into 6faff51 on ismarslomic:master.

@ismarslomic
Copy link
Owner

Could you also update/add unit tests for this change?

@mtebele
Copy link

mtebele commented Sep 19, 2016

Just merge it, I need that fix!

@sam2x
Copy link
Author

sam2x commented Sep 19, 2016

Note that if you use this fix you cannot have animation transition on text. @ismarslomic i don't know how to add unit test, sry.

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.

4 participants