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

issue301 Closes #301 Adds an htmlEntity helper for entities that should ... #417

Merged
merged 1 commit into from
May 1, 2015

Conversation

srmelody
Copy link
Contributor

...be unescaped but do not need to be scaped. We can use the escapeChars helper to handle the smaller set of html escape chars

Please note that I did not commit the dist/ files. My assumption is they get built at release time. I also added a set of the html entities, but it's not exhaustive. I could remove the extras and just add nbsp to the escape codes array.

@@ -17,8 +17,8 @@ test('#unescapeHTML', function(){
equal(unescapeHTML('&'), '&');
equal(unescapeHTML('&'), '&');
equal(unescapeHTML(''), '');
equal(unescapeHTML(' '), ' ');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add tests for all the htmlEntities?

@stoeffel
Copy link
Collaborator

How about adding support for this additional htmlEntities to escapeHtml (except &nbsp) too?

@stoeffel
Copy link
Collaborator

Maybe a list of supported htmlEntities in the readme would be nice for users.

@srmelody
Copy link
Contributor Author

@stoeffel I addressed your great feedback, thanks. Let me know if you see any issues with the updated commit.

@stoeffel
Copy link
Collaborator

If 'helpers/escapeChars' is only used in escapeHtml, the keys and values could be swapped instead of swapping it in the for loop ( I didn't check if it isn't used anywhere else, I'm on my phone 😄). Thanks for your changes.

@srmelody
Copy link
Contributor Author

Yes, I was able to remove the map reversing and explicitly build the escapeChars map once. Good suggestion 😄

@srmelody
Copy link
Contributor Author

Anything else @stoeffel ?

@stoeffel
Copy link
Collaborator

No, I think it looks good. I will check again as soon as I have time and then merge it. Thanks

gt: '>',
quot: '"',
amp: '&',
'#39': "'",
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no need for this. Checkout line 12 in unescapeHtml.

…that should be unescaped but do not need to be scaped. We can use the escapeChars helper to handle the smaller set of html escape chars
@srmelody
Copy link
Contributor Author

@stoeffel I removed the line you called out. Thanks. :)

stoeffel added a commit that referenced this pull request May 1, 2015
issue301 Closes #301 Adds an htmlEntity helper for entities that should ...
@stoeffel stoeffel merged commit 09ba69b into esamattis:master May 1, 2015
@stoeffel
Copy link
Collaborator

stoeffel commented May 1, 2015

Thanks!

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