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.

Sign up for GitHub

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

Jump to bottom

Add directive to update link tag #18

Merged
merged 7 commits into from
Apr 15, 2017
Merged

Add directive to update link tag #18

merged 7 commits into from
Apr 15, 2017

Conversation

khisakuni
Copy link
Contributor

Update link elements

  • Add UpdateLinkDirective to update link
  • Extract logic for updating an attribute into UpdateAttributeService
  • Update example

Kohei Hisakuni and others added 5 commits May 2, 2016 08:57
Add directive to update link tag
0df5e93 
- add new directive
- add service to update attribute of element
- update example
Add log dependency
d8e1d51
Cleanup
dca956b
Fix name error
cd277cb
Add new build
69aa555
@jvandemo
Copy link
Owner

jvandemo commented Jun 6, 2016

@khisakuni — I totally missed this, sorry. This looks great!

Am I correct in assuming that this would also make #20 possible? Thank you!

@khisakuni
Copy link
Contributor Author

@jvandemo Yeah sounds like this issue is covered by this PR

@khisakuni
Copy link
Contributor Author

@jvandemo ping

@jvandemo
Copy link
Owner

@khisakuni — I'll have a look at this today 👍

@jvandemo
Copy link
Owner

@khisakuni — I have finally had a chance to look at this. Great work! I have a couple of questions:

1. How can we target different link elements with same rel?

Due to the nature of link elements, it is sometimes not possible to identify them using only the rel attribute. Suppose you have:

<head>
  <link rel="stylesheet" href="styles1.css" />
  <link rel="stylesheet" href="styles2.css" />
</head>

and:

<update-link rel="stylesheet" href="styles3.css"></update-link>

then we get:

<head>
  <link rel="stylesheet" href="styles3.css" />
  <link rel="stylesheet" href="styles2.css" />
</head>

So the first element is always targeted, which could potentially not be the user's intention.

Would there be a way to target which element you want to update?

2. Support for other attributes?

The link element supports other attributes such as hreflang, see https://developer.mozilla.org/en/docs/Web/HTML/Element/link#Attributes for a complete list.

Would you be able to add support for them too?

I think your PR would be a great addition if both "issues" could be solved.

What do you think?

Thanks!

@khisakuni
Copy link
Contributor Author

@jvandemo Thanks for taking a look!

I'll look into the things you mentioned and keep you posted.

@jvandemo
Copy link
Owner

@khisakuni — Thank YOU for making this happen!

@rolintoucour
Copy link

Hi and thanks for your work. I also need need this function, is it possible to merge it into the trunk?

@jvandemo
Copy link
Owner

@rolintoucour — The current status of this PR is not ready to be merged into master yet as @khisakuni is still working on it. If you need the functionality already, you can manually use the code from this PR to create your own custom library. Thanks!

@khisakuni
Copy link
Contributor Author

@jvandemo sorry it took me a while to get around to this. Added support for specifying a link with an id attribute. So if there are two stylesheet links:

<link rel="stylesheet" href="styles1.css" />
<link rel="stylesheet" href="styles2.css" id="foobar" />

and you have

<update-link rel="stylesheet" href="styles3.css" id="foobar" ></ update-link>

it will query the DOM using the provided id.

This now also supports attributes found here.

<update-link  rel="test" href="http://example.com" hreflang="es"></ update-link>

will update both the href and the hreflang attributes.

Let me know what you think!

@jvandemo
Copy link
Owner

@khisakuni — Fantastic work, this looks great.

I will simulate some scenarios to see if the outcome is as expected.

Thank you for your efforts!

@marcoraddatz marcoraddatz mentioned this pull request Oct 21, 2016
Closed
@jvandemo
Copy link
Owner

@khisakuni — Thank you, this is awesome!

One final question before merging: can you write down the pattern that you use to figure out which element to target? Then we can update the documentation to describe how the elements are matched.

E.g. does it first try to match all attributes? And then also match on ID? What happens if 2 elements have the same ID but different attributes?

It would be great to have a list of all scenario's so we can document everything nicely before merging.

Thanks again for your wonderful contribution!

@simonepri
Copy link
Contributor

@jvandemo , @khisakuni there are some news about the status of this PR ?
Thanks

@jvandemo
Copy link
Owner

@simonepri — We are waiting for @khisakuni to document the targeting pattern before we can merge this PR. Hopefully @khisakuni can chime in. Thank you!

@khisakuni
Copy link
Contributor Author

@simonepri @jvandemo hey sorry guys, been pretty slammed with work. Will try to update tonight.

@jvandemo
Copy link
Owner

@khisakuni — No problem, take your time. We appreciate all the effort you put into this 👍

@cesarvarela
Copy link

news about this guys?

@alee046
Copy link

alee046 commented Mar 23, 2017

Same, any updates?

@jvandemo
Copy link
Owner

@cesarvarela, @alee046 — Unfortunately, this can't be merged without proper documentation. Once the documentation is available, we can test proper behavior and merge if okay.

@khisakuni
Copy link
Contributor Author

@jvandemo Apologies that I've neglected this for a while! I updated the documentation to reflect the changes in this PR.

@jvandemo jvandemo merged commit b65511a into jvandemo:master Apr 15, 2017
@jvandemo
Copy link
Owner

@khisakuni — Thank you for your great work, much appreciated!

Your changes have been merged and released as v2.0.0. 👍

@simonepri
Copy link
Contributor

@khisakuni Thank you

@simonepri
Copy link
Contributor

@jvandemo I think you forgot to push it to bower:
image

@jvandemo
Copy link
Owner

@simonepri — Thank you, great catch. I re-created the tag.

Can you please try again and verify if it works as expected? Thank you!

@khisakuni khisakuni mentioned this pull request May 17, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@khisakuni @jvandemo @rolintoucour @simonepri @cesarvarela @alee046