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

warn: allow user to preview/change edit summary before warning #1818

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

m-etroo
Copy link

@m-etroo m-etroo commented Jul 12, 2023

Upon opening the warning dialog, the user has the option to modify the edit summary that will eventually be used.

Currently, the edit summary (including any edits by the user) will be replaced entirely when a new warning is selected or when the linked page is changed. Not sure if this needs to be changed down the line to try to preserve the user's edits.

Resolves #1803

image

Copy link
Member

@NovemLinguae NovemLinguae left a comment

Choose a reason for hiding this comment

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

A pretty complex first patch. Nice work.

Can we delete the <legend>Warning information</legend>? I don't think it makes much sense to have linked page outside of it, and edit summary, optional message, and preview inside of it.

I think we can also move "edit summary" to above "linked page". Visually I'd like linked page and optional message grouped, since those both get turned off sometimes. Would be weird to have edit summary in the middle of that.

Copy link
Member

@siddharthvp siddharthvp left a comment

Choose a reason for hiding this comment

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

I'd put the edit summary at the bottom of the form, since that seems the least important input here.

Twinkle.warn.callback.set_tentative_edit_summary = function twinkleWarnCallbackSetTentativeEditSummary(e) {
var selected_main_group = e.target.form.main_group.value;
var selected_template = e.target.form.sub_group.value;
var messageData = $(e.target.form.sub_group).find('option[value="' + $(e.target.form.sub_group).val() + '"]').data('messageData');
Copy link
Member

Choose a reason for hiding this comment

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

There has to be a less error-prone (what if the value contains special chars?) and more elegant way to do this.

Maybe something like $(e.target.form.sub_group).find('option:selected') ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

warn: make edit summary modifiable via a text box
3 participants