-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
Replace sign_in.js by webcomponent #4023
Conversation
This pull request introduces 1 alert when merging bdabab2 into 7231018 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 999109a into 7231018 - view on LGTM.com new alerts:
|
app/assets/javascripts/util.js
Outdated
function htmlEncode(str) { | ||
return String(str) | ||
.replace(/&/g, "&") | ||
.replace(/</g, "<") | ||
.replace(/>/g, ">") | ||
.replace(/"/g, """); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit hesitant about adding this function because it's incomplete. (and in combination with the unsafeHTML function where it's called). Is there an alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a way without the unsafe html that also marks the search results. And as the institution names are user input we need some kind of html encoding.
This method is an often suggested solution:
- https://stackoverflow.com/questions/6234773/can-i-escape-html-special-chars-in-javascript
- https://stackoverflow.com/questions/24816/escaping-html-strings-with-jquery
Some indeed change more keys. I will add those. They are more relevant when the string is used within html tags. But for a general util method it should cover all cases.
There is weirdly no standard javascript method for this. JQuery and lodash provide solutions, but we dont want to include those.
This might be a better alternative:
var text = document.createTextNode(html);
var p = document.createElement('p');
p.appendChild(text);
return p.innerHTML;
}```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request replaces sign_in.j by a webcomponent. This is part of our js modernization.
I also added tabcompletion and higlighting matches to the datalist component. Which has a wider impact.
Old
New
Part of #3590