-
Notifications
You must be signed in to change notification settings - Fork 143
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
tag: allow entering a reason #889
Conversation
I think it's fine to have all the time. It's at the bottom so out of the way, and I think it'd be nice if folks used it when adding as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me, only thing that's missing AFAICT is a little whitespace awareness.
I can't add a comment to code outside the diff, but I also think we should change pntPage
's use of params.reason
to params.pntreason
or something; this and that shouldn't collide, but for clarity's sake in the code they should differ. Alternatively, this could be params.removeReason
or something.
modules/friendlytag.js
Outdated
@@ -2014,6 +2023,7 @@ Twinkle.tag.callback.evaluate = function friendlytagCallbackEvaluate(e) { | |||
case 'article': | |||
params.tagsToRemove = form.getUnchecked('alreadyPresentArticleTags') || []; | |||
params.tagsToRemain = form.getChecked('alreadyPresentArticleTags') || []; | |||
params.reason = form.reason.value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a .trim()
for whitespace. Could be done above (line 1295) in the if
but here should be fine?
params.reason = form.reason.value; | |
params.reason = form.reason.value.trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meh, I guess it's time for Morebits.quickForm.getInputData
to finally see the light of the day, which should get rid of needing to do all this by hand!
Allow entering a reason to be appended in the edit summary. Primarily useful when removing tags.
don't really need to get/set callback parameters here; `params` can be directly accessed within the load callback as a closure.
handled in a 2nd commit, a bit differently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaner way to handle PNT, seems fine!
Allow entering a reason to be appended in the edit summary.
Unlike adding of tags, it is typically expected that a reason be given while removing tags. Truth be told, the untag feature (#485) remains incomplete without this. I don't think I have seen the untag feature being used any more than 1-2 times.
Demo of usecase: Special:Diff/945510112
Currently the reason field is shown all the time. I'm open to feedback on this. Would it be better to show the reason field only when one or more tags have been de-selected?