search

troxler / vue-headful

Set document title and meta tags with Vue.js
MIT License
229 stars 17 forks source link

A possible major update. #2

Closed Raiondesu closed 6 years ago

Raiondesu commented 6 years ago

This PR also closes #1 as there's no need for Object.assign anymore.

@troxler, please, consider pushing this to npm, as I and some other people need it for work.

P.S. It's a great and a very simple package compared to other alternatives. And I really want to make it better.

kaskar2008 commented 6 years ago

@troxler Pls, consider to review this. Plugin instead of the component is a good idea. I'd like to see these changes on the NPM as well.

Raiondesu commented 6 years ago

Ok, due to a lack of attention from the repo's owner - @troxler, I'm gonna make my fork as a separate npm package, because I need it for work AND there's literally no original code left.

If upon the moment of publication of my fork, this PR will not be reviewed - I'll close it due to its obsolescence.

Until then - all of my changes will be available here.

troxler commented 6 years ago

Thank you for contributing, especially in such a big way :)

I have reviewed your changes and I am afraid but I cannot merge your request. It changes the project completely, including the API for the end-user. Thus, it would break the backward compatibility and lose one of the main reasons I started this project in the first place: A way to change the title and meta tags from the view. There are already other projects that allow changing it from the controller, though.

I'm gonna make my fork as a separate npm package, because I need it for work AND there's literally no original code left.

That is actually what I wanted to suggest, too. I would gladly link to it from the headful Extensions section. I will also make some comments on your code, maybe that helps.

Fixed bugs.

Can you please specify what you are referring to?

Ok, due to a lack of attention from the repo's owner

Sorry, I did not know that we have an SLA ;)

Raiondesu commented 6 years ago

Ok, got it. :) I'll be out making vue-simple-headful then.

I will close this now and propose a PR to headful README when I finish working on my own fork, if you don't mind.

troxler commented 6 years ago

Fixed bugs.

Can you please specify what you are referring to?

@Raiondesu Ping

Raiondesu commented 6 years ago

@troxler, the bugs I fixed in my fork are now irrelevant, after you updated your implementation. Honestly, I don't really remember that much about the bugs that were fixed. I think it was something connected to the watch bindings, because of the complicated logic you had earlier. And since watch: { '$props' } is a much simpler and convenient solution - there's nothing to be worried about here.