Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fluff: Allow for a list of custom edit summaries #1283

Closed
wants to merge 4 commits into from

Conversation

TheTVExpert
Copy link
Contributor

@TheTVExpert TheTVExpert force-pushed the patch-12 branch 2 times, most recently from f153779 to 65b7b50 Compare January 20, 2021 21:58
@siddharthvp
Copy link
Member

siddharthvp commented Jan 22, 2021

Haven't tested, but this sounds pretty good to me. I had the same idea in mind to resolve #980. Use of a <datalist> would reduce the number of form fields from 2 to 1, but here I suppose it's better not to use datalist as the form would look too empty. Also datalist isn't supported in Safari <12, though not sure if we care much about those browsers.

It would be good to provide some some defaults in the customReversionSummaryList. Try copying from RedWarn.

@TheTVExpert
Copy link
Contributor Author

@siddharthvp I've added some defaults for the customReversionSummaryList.

Copy link
Collaborator

@Amorymeltzer Amorymeltzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I've a few comments with a quick first pass. It's a new use of Morebits.simpleWindow for us, but it seems to work pretty nicely!

Regarding #980, we had a discussion there @siddharthvp about not stopping the event loop. I never played around too deeply, is that a concern here?

modules/twinklefluff.js Show resolved Hide resolved
modules/twinklefluff.js Outdated Show resolved Hide resolved
modules/twinklefluff.js Outdated Show resolved Hide resolved
modules/twinklefluff.js Outdated Show resolved Hide resolved
modules/twinklefluff.js Outdated Show resolved Hide resolved
@@ -69,6 +69,7 @@ Twinkle.defaultConfig = {
markRevertedPagesAsMinor: [ 'vand' ],
watchRevertedPages: [ 'agf', 'norm', 'vand', 'torev' ],
watchRevertedExpiry: 'yes',
customReversionSummaryList: [{value: 'Unsourced addition', label: 'Unsourced'}, {value: 'Unexplained removal of content', label: 'Removal of content'}, {value: 'Potential copyright violation', label: 'Copyright violation'}, {value: 'Disruptive editing', label: 'Disruptive editing'}],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this. Would it make more sense to make this list be empty and allow users to provide it themselves, then offer a few options (such as these?) in addition to that list? Kind of like the other customizations (warn/tag/welcome templates). It'd be annoying if you want to remove one, but I think a dropdown would help with that (though see other comment).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed this looks a bit silly. Not so much agreed on the solution. I see that twinkleconfig is missing a "plainlist" data type that would have been the natural solution here. It might be worth adding that just for this usecase.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd have to prepopulate it with something, no?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two things to discuss here:

One is how to store the data. A plainlist would be nice, but I don't think customlist is a huge hindrance. The latter might make the display easier to parse for folks if there are multiple options. So maybe "sources: removed reliable sources" and "sources: needs reliable sources" or something. A plainlist does seem simpler given we just need one string of text.

The other is how to incorporate defaults, which I'm more concerned about. With this setup, if someone customizes the list, they are forever left out of any changes we make unless they hit "reset, view the default options, then re-add from memory what they wanted; that's not good. Instead we can do what is done elsewhere, namely include the basic uses in the module, and then add any items from customReversionSummaryList on top of that. The advantage of the former is that users could cull the defaults, though.

modules/twinklefluff.js Show resolved Hide resolved
@siddharthvp
Copy link
Member

Regarding #980, we had a discussion there @siddharthvp about not stopping the event loop. I never played around too deeply, is that a concern here?

No I just meant that a prompt can't be easily swapped for a simpleWindow; it requires quite a bit of restructuring – which has been done here.

@TheTVExpert
Copy link
Contributor Author

@Amorymeltzer Done.

@Amorymeltzer Amorymeltzer added this to the Next update milestone Jan 31, 2021
@Amorymeltzer Amorymeltzer removed this from the March 2021 update milestone Feb 28, 2021
@NovemLinguae NovemLinguae added the stale PR closed because it has had no action in years. Feel free to reopen if work is planned. label Feb 21, 2023
@NovemLinguae
Copy link
Member

We are closing old PRs that haven't been worked on in a couple years. Please feel free to reopen if you plan to work on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature request Module: config Module: fluff Module: twinkle The twinkle.js global gadget file stale PR closed because it has had no action in years. Feel free to reopen if work is planned.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants