Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Add custom attributes to linky #12558

Closed
wants to merge 1 commit into from

Conversation

stianjensen
Copy link
Contributor

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

@googlebot
Copy link

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!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@stianjensen
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@lgalfaso
Copy link
Contributor

A test would be nice

@Narretz
Copy link
Contributor

Narretz commented Aug 20, 2015

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?

@stianjensen
Copy link
Contributor Author

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.
I guess you still want to keep the target parameter as a dedicated one, just for the sake of backwards compatibility?

@Narretz
Copy link
Contributor

Narretz commented Aug 20, 2015

@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 3rd parameter is either a map of attributes, or a function that receives the url as a parameter and returns a map.
  • if the 2nd parameter is set, and the map contains the target attribute, it overrides it

@stianjensen
Copy link
Contributor Author

if the 2nd parameter is set, and the map contains the target attribute, it overrides it

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?

@stianjensen stianjensen changed the title Add optional nofollow parameter to linky Add custom attributes to linky Aug 20, 2015
@stianjensen
Copy link
Contributor Author

I updated the pull request with support for custom attributes now 😄

html.push('<a ');
if (angular.isDefined(target)) {
if (typeof attributes === 'object') {
Copy link
Contributor

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

@stianjensen
Copy link
Contributor Author

Updated

html.push('<a ');
if (angular.isDefined(target)) {
if (angular.isObject(attributes)) {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@Narretz Narretz modified the milestones: 1.5.x - migration-facilitation, Backlog Aug 31, 2015
@Narretz Narretz self-assigned this Sep 19, 2015
@@ -137,8 +163,19 @@ angular.module('ngSanitize').filter('linky', ['$sanitize', function($sanitize) {
}

function addLink(url, text) {
var key;
var computedAttributes = {};
Copy link
Contributor

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?

Copy link
Contributor Author

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!

@petebacondarwin
Copy link
Contributor

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
@stianjensen
Copy link
Contributor Author

ping

@Narretz
Copy link
Contributor

Narretz commented Oct 9, 2015

The generated HTML is sanitized before it is inserted. I'll add a test for that.

@Narretz
Copy link
Contributor

Narretz commented Oct 9, 2015

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.

Narretz pushed a commit to Narretz/angular.js that referenced this pull request Oct 9, 2015
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
Narretz pushed a commit to Narretz/angular.js that referenced this pull request Oct 9, 2015
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
Narretz pushed a commit to Narretz/angular.js that referenced this pull request Oct 9, 2015
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
@Narretz Narretz closed this in 06f002b Oct 9, 2015
@Narretz
Copy link
Contributor

Narretz commented Oct 9, 2015

Landed as 06f002b
thx @stianjensen for working on this!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants