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.

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

Themes: Get rid of Helmet #3795

Closed
ockham opened this issue Mar 5, 2016 · 4 comments
Closed

Themes: Get rid of Helmet #3795

ockham opened this issue Mar 5, 2016 · 4 comments

Comments

@ockham
Copy link
Contributor

ockham commented Mar 5, 2016

We should really get rid of Helmet, as it's more hassle than benefit.

Rationale: For most pages we want to give title and meta tags, we'll have to fetch some data from Redux (e.g. theme name for a theme sheet). From there, we have to add those data to Helmet; rewind Helmet right after renderToString to get the data back (and not forget to cache them alongwith the rendered markup!); and pass them to index.jade to have them rendered in their appropriate places.

Or we just get them from Redux, and pass them to index.jade.

@ockham
Copy link
Contributor Author

ockham commented Mar 5, 2016

We might want to turn the setTitle() Flux action in lib/screen-title into something Reduxy for us to use -> #3796

@ockham
Copy link
Contributor Author

ockham commented Jun 22, 2016

Now that there is the DocumentHead component, I think that's what we'll want to use. Internally, this still relies on Flux stuff (invoked by Redux actions), so we'll need to change that before we can use DocumentHead on the server.

@tomas-andr
Copy link

Ok, I took a look at the source code, if I understand correctly:

  • DocumentHead is trying to be the new Reduxy way of managing the document head (title, meta, ...)
  • the real work is still being done by lib/screen-title, which relies on Flux, the meta tags are not really supported
  • Helmet is there mostly for the first (probably server-side) render anyway

I propose to merge DocumentHead and lib/screen-title into one Reduxy component, and also use this one in Head (currently only a Helmet wrapper)

@ockham
Copy link
Contributor Author

ockham commented Jul 4, 2016

Sounds quite good! You're right with your diagnosis -- one note on the suggested therapy:

I propose to merge DocumentHead and lib/screen-title into one Reduxy component

Not sure I'm reading you correctly here, but I wouldn't merge everything into a component. Note that DocumentHead already uses Redux actions, so we shouldn't unnecessarily introduce new ones. Instead, let's make sure that any side effects currently handled by the Flux stuff are instead handled in a Reduxy way. And as for server-side rendering, it will ideally be able to just use the initial Redux state.

You might find some more ideas floated around in issues around DocumentHead by @aduth.

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

No branches or pull requests

3 participants