search

musically-ut / lovely-forks

💚 🍴 Show notable forks of GitHub repositories under their names.
Mozilla Public License 2.0
603 stars 40 forks source link

Modernize extension #38

Closed fregante closed 7 years ago

musically-ut commented 7 years ago

w00t! This is a large rewrite! Thanks for bringing the extension to the 21st century! :)

I'll test it out with Firefox later today. I'm assuming that you've done Chrome already. I'll leave some comments on the PR as well.

musically-ut commented 7 years ago

Adding a note for later review: Userscript version.

fregante commented 7 years ago

Yeah, about the userscript version. Are you sure you want to maintain that? Especially manually?

I've been asked to port my extensions to userscripts before but userscripts aren't as capable and I don't really care of userscripts (which need a base extension to be executed in the first place, like tampermonkey and greasemonkey)

musically-ut commented 7 years ago

There is an argument to be made about ubiquity of userscripts: they can run on IceMonkey/Opera/Safari, etc almost out of the box.

However, I also see that the Userscript may restrict one to an older version of JavaScript. Perhaps @yfdyh000 could also share some of his motivations for making a Userscript version in the first place?

fregante commented 7 years ago

Sure, if you want to support the dozen of users on those three browsers combined no one will stop you, I just don't think it's worth the effort. Opera supports Chrome extensions anyway.

I'll add: it's not about javascript versions but about the web-ext API, like the more flexible Storage API

yfdyh000 commented 7 years ago

I don't think the userscript block this review, you can keep it as a legacy version. Create userscript because the original extension is almost just a userscript, missing meta only, and I hope it can be more easily customized and improved.

yfdyh000 commented 7 years ago

Also, I think this updates can be modified to userscript, but there is no significant power to do so for now. If we want to make the user configuration / options, the userscript will be a nice prototype and lightweight, like const optionA = true; instead of an options interface.

musically-ut commented 7 years ago

I've merged this in and have left the userscript as the legacy version.

However, I'll release the next version after I've added the preference for the indentation.

Thanks @bfred-it!