Closed brunolemos closed 6 years ago
This is awesome looking forward to the merge.
I'm open to having some of the features configurable, but not all (I've commented inline on the ones I don't think it's worth having configurable). Having all features toggle-able leads to a maintenance nightmare, as some features might depend on others.
Thanks for working on this @brunolemos! 🙌
@sindresorhus Can you consider two options to be configurable? I have added the relevant comments inline. It's really important for me to have those for valid reasons.
The point of it being configurable is to let people configure it the way they want. I promote tweets for my company and I want to be able to see how they look. If this is not configurable then I can't use this extension and all the effort in this PR is back to square 1.
Yeah, the reason why I made all configurable is because I also believe this. People should be able to only enable what they want. The extension should not get in the way.
I've known this extension for months and I was not using it because of some things it was disabling; making it configurable solves for everyone.
Having all features toggle-able leads to a maintenance nightmare
Maybe on other kinds of open source projects but this one is straightforward; Maybe we could enable all customizations and only disable some in the future if this actually starts to happen
as some features might depend on others.
This should not happen; the feature function should enable everything that is necessary for it to work independently of the others
@brunolemos @ahmadawais You're seeing this from the perspective of a user. From the perspective of a maintainer, I'm the one that in the end will be stuck dealing with bugs and feature incompatibilities. We should be able to find a middle-ground.
Yeah, the reason why I made all configurable is because I also believe this. People should be able to only enable what they want. The extension should not get in the way.
There's such a thing as being too configurable. It's all about the balance.
@sindresorhus I have the utmost respect for you as an open sourcerer. You're one of the few developers whose work encouraged me to be a full-time open source developer.
I want to prefix this message with that. I also will respect your decisions here and can only offer my opinion as a user as I know and already maintain about 400 repos. I know the pain of maintaining features that you didn't want in the first place.
Looks like you're OK with one of my suggestions Ok, leave this configurable then.
cc: @brunolemos from my end, all I'm left with is the option of Hide Twitter bird logo
being configurable. Is there anything we can do in this PR which can make you allow that one extra configuration?
If not, I guess @brunolemos then we'll have to respect @sindresorhus's decision and maybe we can maintain a configurable fork or something.
Looking forward, peace! ✌️
@sindresorhus All requested changes are in! And 6 configurations were disabled*
*By disabled I mean hidden. It's still possible to inspect the Options screen and unhide them. But that would be considered as unmaintained and almost no one would know about this.
Sounds cool? 🙌
@brunolemos Yup. Can you fix the merge conflict?
// @jorgegonzalez @filipekiss Can you help review and test this?
All options seem to be hidden for me.
@jorgegonzalez: All options seem to be hidden for me.
Thanks for trying it out!
The root cause for this (it doesn't load the scripts on development mode) is:
Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'self' blob: filesystem: chrome-extension-resource:".
You can fix this by going to package.json
and changing:
-"watch": "webpack --mode=development --watch",
+"watch": "webpack --mode=production --watch",
I didn't include this change on this PR but let me know if I should. There is probably a better way to fix this.
@jorgegonzalez let me know if the configurations work well for you.
@sindresorhus: Can you fix the merge conflict?
Rebased with master. 👍
Thanks @brunolemos :)
Pardon my ignorance but has it not been released to the Chrome store yet or do I need to do something via Inspect to make the options visible.
I still see none 🤔
Adds ability to enable or disable features, because each person has it's own preferences 💙
Fixes #38 Fixes #128