-
Notifications
You must be signed in to change notification settings - Fork 27.4k
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
3241947
to
34ab512
Compare
A test would be nice |
Would it make sense to make the third parameter more powerful - i.e. allowing it to add arbitrary attributes by passing a map or a function that returns a map? |
I guess that would be even better. I'm sure many would like the option of adding classes to links for instance. I'll take a stab at changing the parameter. |
@stianjensen Great! Yes, the target parameter should stay. I imagine the API looking something like this (I haven't checked with the current code though):
|
The 2nd parameter overrides the target attribute from the map, or the other way around? Also, am I correct that there is no way you could provide a value for the 3rd parameter without also providing a value for the 2nd parameter? |
34ab512
to
56f0c37
Compare
I updated the pull request with support for custom attributes now 😄 |
html.push('<a '); | ||
if (angular.isDefined(target)) { | ||
if (typeof attributes === 'object') { |
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.
We can use angular.isObject
56f0c37
to
9b4237e
Compare
Updated |
html.push('<a '); | ||
if (angular.isDefined(target)) { | ||
if (angular.isObject(attributes)) { |
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.
Can this code duplication be prevented using something like this?
var key;
var computedAttributes = {};
if (angular.isFunction(attributes)) {
attributes = attributes(url);
}
if (angular.isObject(attributes)) {
computedAttributes = attributes;
for (key in computedAttributes) {
html.push(key + '="' + computedAttributes[key] + '" ');
}
}
This would also check if the return value from the attributes
function is an object.
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 guess that's a good idea. At first I didn't want to reassign the attributes
variable, but the extra check for the return value of attributes as a function, is a compelling case for doing it like this.
9b4237e
to
3530228
Compare
@@ -137,8 +163,19 @@ angular.module('ngSanitize').filter('linky', ['$sanitize', function($sanitize) { | |||
} | |||
|
|||
function addLink(url, text) { | |||
var key; | |||
var computedAttributes = {}; |
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.
Do we need to use a new variable computedAttributes
here?
Can't we just assign attributes = {}
in an else block starting at line 177?
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.
Good idea, I'll do that!
Other than those two questions, it LGTM |
Optional extra attributes may be defined either as: - a map of attributes and values - a function that takes the url as a parameter and returns a map
3530228
to
4301dc1
Compare
ping |
The generated HTML is sanitized before it is inserted. I'll add a test for that. |
Looks like IE and FF insert the attributes in a different order! (see Travis failures). Looks like we'll have to change these expects to be more robust. |
Optional extra attributes may be defined either as: - a map of attributes and values - a function that takes the url as a parameter and returns a map Closes angular#12558
Optional extra attributes may be defined either as: - a map of attributes and values - a function that takes the url as a parameter and returns a map Closes angular#12558 Closes angular#13061
Optional extra attributes may be defined either as: - a map of attributes and values - a function that takes the url as a parameter and returns a map Closes angular#12558 Closes angular#13061
Landed as 06f002b |
Optional extra attributes may be defined either as: