search

kadirahq / meteor-dochead

Isomorphic way to manipulate document.head for Meteor apps
MIT License
131 stars 17 forks source link

Added a setMeta method (on client). #13

Closed krstffr closed 9 years ago

krstffr commented 9 years ago

Added a setMeta method which unlike addMeta does not append new meta tags on each call.

For example: if you run DocHead.addMeta({ name: "description", content: "Some content" }) you will end up with TWO <meta name="description"> tags in your <head>, which you now can avoid by using the setMeta method (which makes sure there is always at most one tag at a time).

Only added the method on client since it don't make (as much) sense on the server to run the addMeta more than once.

krstffr commented 9 years ago

This is what I would like to avoid (here I'm using the addMeta method when navigating from one page to another):

skarmavbild 2015-10-02 kl 15 32 10

arunoda commented 9 years ago

Looks good to me. Implement this in the server as well.

krstffr commented 9 years ago

Hi @arunoda!

I guess that would require something like a new removeFromHead method in the flow-router-ssr packages?

https://github.com/kadirahq/flow-router/blob/ssr/server/ssr_context.js

arunoda commented 9 years ago

In the SSR you don't need to remove. Just call addMeta. On 2015 ඔක් 2, සිකු at ප.ව. 7.52 Kristoffer Klintberg < notifications@github.com> wrote:

Hi!

I guess that would require something like a new removeFromHead method in the flow-router-ssr packages?

https://github.com/kadirahq/flow-router/blob/ssr/server/ssr_context.js

— Reply to this email directly or view it on GitHub https://github.com/kadirahq/meteor-dochead/pull/13#issuecomment-145035752 .

krstffr commented 9 years ago

OK, I'm not entirely sure how the ssr side of this all works, but I tried adding the method to the server and a test for it as well. Please look this over though as I'm not sure it's correctly implemented! @arunoda

krstffr commented 9 years ago

Sorry, forgot to remove some code, new commit in a minute! @arunoda

krstffr commented 9 years ago

Hey @arunoda, are you still waiting for me to do something or is everything ready for a merge? :) Thanks!

arunoda commented 9 years ago

Actually, We didn't had this issue and I waited a bit for look at how we did this. Here's how:

For every route change we call DocHead.removeDocHeadAddedTags() to clean them.

Frankly, having something like setMeta is interesting, but since we can't do it inside the server for now, I'll hold this. We can do this, once we figure out a way to do this inside the server.

krstffr commented 9 years ago

Oh, i did not know about that method, cool!

arunoda commented 9 years ago

And I forgot about it too :)

arunoda commented 9 years ago

May be better to add to README.

krstffr commented 9 years ago

Would be great!

krstffr commented 9 years ago

Seem to work just like expected!

Should I remove this pull request?

arunoda commented 9 years ago

Yep. Let's close this. On 2015 ඔක් 6, අඟහ at ප.ව. 2.51 Kristoffer Klintberg < notifications@github.com> wrote:

Seem to work just like expected!

Should I remove this pull request?

— Reply to this email directly or view it on GitHub https://github.com/kadirahq/meteor-dochead/pull/13#issuecomment-145795705 .