toolbox-team / reddit-moderator-toolbox-legacy

LEGACY VERSION do not use
http://www.reddit.com/r/toolbox
Apache License 2.0
66 stars 40 forks source link

Add comment locking functionality #871

Closed CrustyJew closed 5 years ago

CrustyJew commented 5 years ago

This is native in the Redesign so the functionality doesn't have to be ported over and can be left to die in this branch.

Stupidly simple module that just adds a "lock" button after "distinguish" that locks or unlocks a comment

creesch commented 5 years ago

A few point:

CrustyJew commented 5 years ago

It probably could be rolled in to better buttons, but that seems more concerned with changing/fixing functionality of existing mod buttons. It's more of a new "feature" like the nuke button is even though it is much smaller. Is there any major drawbacks to having it in its own module?

Like I mentioned on Reddit I only changed the version name to be able to find it easier locally, that can get set to whatever. I bumped the minor version since it was added functionality, but if you'd prefer it be a patch version that's fine, I can change it in the pull request or you can change it after, whichever you prefer.

I'm not seeing the button added on subs I'm not a mod of. Is it a specific page you are seeing it on?

creesch commented 5 years ago

Is there any major drawbacks to having it in its own module?

Usability as far as settings go and also performance as you now query for comments separately from other modules. However as I already said on reddit this repo will eventually be archived as we move all functionality in one toolbox version (no worries, everything will remain working on old and new reddit) so this isn't that big of a deal to me.

but if you'd prefer it be a patch version that's fine, I can change it in the pull request or you can change it after, whichever you prefer.

That would be my preference, we usually only change the version just before release in a seperate commit

I'm not seeing the button added on subs I'm not a mod of. Is it a specific page you are seeing it on?

No that was just me doing code review on my phone and missing the obvious check you implemented. My bad.

CrustyJew commented 5 years ago

Fixed the version number. For ease of porting and not having to worry about it in any other version, I'd say this can safely be left as its own module. I was doing some performance testing and didn't see any major impacts from it.

creesch commented 5 years ago

Alright, we talked it over a bit on discord and decided we wanted to have it as part of betterbuttons. So in this commit https://github.com/toolbox-team/reddit-moderator-toolbox/commit/ba627b27c4faeec51f30347e7fd97f706930470b I basically took most of your code and transplanted it over there with a few adjustments.

So I am going to close this merge request now, but still thank you for the code contribution as we did end up using it.