freshbooks / ember-responsive

Easy responsive layouts with Ember
https://freshbooks.github.io/ember-responsive/
MIT License
40 stars 19 forks source link

auto-injection of media service into components breaks ember-cli-meta-tags #99

Closed bcardarella closed 6 years ago

bcardarella commented 7 years ago

Because the media service auto-injects into all components: https://github.com/freshbooks/ember-responsive/blob/master/addon/initializers/responsive.js#L11

ember-cli-meta-tags has an attributeBinding for media properties: https://github.com/ronco/ember-cli-meta-tags/blob/master/addon/components/head-tag.js#L18

This results in invalid tags being rendered:

screen shot 2017-07-21 at 9 44 24 am

and we believe this is resulting in Google not parsing our tags properly

We can just override the initializer but I feel like these addons should place nicely with one another out of the box

alexBaizeau commented 7 years ago

IMO injecting the media service is a bit aggressive, we should stop doing this.

The fact that media is a very common word just make it worse

bcardarella commented 7 years ago

I tend to agree. It would be easier if I can just opt-in to whichever controller/route/component/view/etc... needs the service rather than it being all

k-fish commented 7 years ago

@bcardarella we use blueprints in the newer versions to generate the initializer in the app specifically so that people can choose how they want the service injected. https://github.com/freshbooks/ember-responsive/blob/master/blueprints/ember-responsive/files/__root__/initializers/responsive.js (should land in app/initializers/responsive.js if you've run generate or added the addon since 2.0).

I have considered that our defaults might be excessive, the next breaking version I'll likely include less. Suggestions?

bcardarella commented 7 years ago

@k-fish I would suggest removing the initializer and updating the README to suggest people inject the service directly into the module they require it

k-fish commented 7 years ago

Yeah that's reasonable and was something I had considered. I left it as a generated initializer so existing users and user completely new to Ember had less friction, but I agree it pollutes the app.

k-fish commented 6 years ago

3.x is no longer auto-injected. Closing.