Refactor certain modals to `confirm_dialog` module #18629

aryanshridhar · 2021-05-27T16:45:05Z

Refactored multiple UI modals to use the confirm_dialog module. Essentially, converted the ones that presented the user with a simple 'Cancel'/'Confirm' type question.

These includes:

delete_topic -- 3c22739
subscription_invites_warning -- 8ef944e
revoke_invite -- 800cfe7
resend_invite -- 0416203
user_deactivation -- d7f0147
stream_deactivation -- 52d8589
realm_deactivation -- 82b300a

Maybe @sumanthvrao can have a look at it :)

timabbott · 2021-05-27T20:58:30Z

static/templates/settings/resend_invite_modal.hbs

-</div>
+<p>
+    {{#tr}}
+    Are you sure you want to resend the invitation to {{email}}?


We want the previous formatting to decorate the email address.

timabbott · 2021-05-27T20:59:24Z

static/js/settings_invites.js

-            },
+        confirm_dialog.launch({
+            parent: modal_parent,
+            html_heading: $t_html({defaultMessage: "Resend invitation to {email}"}, {email}),


The heading might be better as just "Resend invitation", rather than showing the email address twice (here and in the body of the template).

Cool! Will add a separate commit for it.

Maybe same goes for "Revoke invitation" as well ?

timabbott · 2021-05-27T21:00:06Z

static/templates/settings/deactivation_user_modal.hbs

-</div>
+<p>
+    {{#tr}}
+    By deactivating <strong>{{username}}</strong> &lt;{{email}}&gt;, they will be logged out immediately.


The #*inline z-user syntax is important for the translation experience (and is why you're getting CI failures).

Please preserve that formatting of the translated strings.

timabbott · 2021-05-27T21:03:17Z

@aryanshridhar thanks for working on this! I'm excited about this cleanup, since it lets us delete a ton of duplicated HTML. Can you fix the issues I noted, and post before/after screenshots for every modal to confirm that they're all visually identical to before the migration. (I guess except for the one I suggested a small wording change to).

(BTW, is there something we could have documented better to explain why the z-link thing is important? E.g. maybe a modified error message?)

Once you have that (and have done a self-review to confirm this has the exact same logic as before; and/or any changes are clearly explained in the commit message or done in prep commits), it'd be great if you can post in #frontend and get a code review on it from another contributor.

aryanshridhar · 2021-05-28T18:52:59Z

Thanks @timabbott, Yea, I guess documenting the purpose of z-link would definitely help. Maybe within https://zulip.readthedocs.io/en/latest/translating/translating.html#frontend-translations ?

Screenshots:

Before

After

Before

After

Before

After

Before

After

Before

After

Before

After

Before

After

timabbott · 2021-05-28T22:50:45Z

The z-link thing is covered in the last section of this page: https://zulip.readthedocs.io/en/latest/translating/internationalization.html#frontend-translations; any idea what we could do to make it more likely you'd have noticed it?

timabbott · 2021-05-28T22:51:08Z

@sahil839 are you up for doing a review / QA pass on this?

sahil839 · 2021-05-31T07:26:06Z

static/js/stream_create.js

                stream_name,
                count: principals.length,
            });
-            $("#stream-creation").append(invites_warning_modal);


Any reason for changing the parent element of this modal?

Added a comment here.

sahil839 · 2021-05-31T07:48:22Z

static/js/settings_invites.js

-        $("#do_revoke_invite_button").off("click");
-        $("#do_revoke_invite_button").on("click", do_revoke_invite);
+        const modal_parent = $("#admin_invites_table");
+        const html_body = render_settings_revoke_invite_modal(ctx);


Here also, why the modal parent is changed. And if there is a reason we want to use this, we can simply remove the #revoke_invite_modal_holder element.

sahil839 · 2021-05-31T11:17:42Z

I tested it locally and it works fine.
I am not sure whether the changes in parent elements are required are not and not sure how we decide the parent elements, but most of those changes look fine. But we should remove the elements that were being used as parent before if they are not required now.
Will wait for other suggestions for this.

aryanshridhar · 2021-05-31T17:08:52Z

I guess its better if I modify the parent elements as they were earlier since they were specifically designed for it. Thoughts @sahil839.
It's also valid that we can remove unnecessary elements if the current changes looks good.

sahil839 · 2021-06-01T17:59:35Z

There is no problem with current changes as they look and behave in the same way, but am just not sure how these parent elements are/were decided.

timabbott · 2021-06-03T00:05:05Z

I think it's worth removing unnecessary elements. We probably mostly want a policy around parent elements for modals to make that not a thing we need to think about; possibly there should just be a single fixed parent element for all modals in the app, since I don't see why it'd matter what their parent is.

timabbott · 2021-06-03T00:12:30Z

static/templates/settings/deactivation_user_modal.hbs

+<p>
+    {{#tr}}
+        By deactivating <z-user></z-user>, they will be logged out immediately.
+        {{#*inline "z-user"}}<strong>{{username}}</strong> &lt;{{email}}&gt;{{/inline}}


You appear to have removes some of the HTML here for styling the username and email; why?

As a sidenote, it's really harmful to have hidden functional changes like this that are not explained clearly in the commit message; only the most watchful reviewer will notice, and it can lead to significant bugs/regressions.

(The last 3 commits all have this issue with either the content or heading).

Umm, Apologies if I am missing something out here @timabbott.
By "HTML here for styling", I guess you meant the CSS classes like - user_name and email ?
If that's the case, the classes like user_name and email are added so that later they can be populated with respective usernames and email.
Looking at the browser inspector, they don't seem to be really adding any styling for the same.

Also, this comment contains the before/after screenshots within it.

Ahh, great; I think the key thing here is that it's really important to explain that sort of reasoning in the commit message, because it's not at all obvious that this is what those are for (though in retrospect it totally makes sense).

timabbott · 2021-06-03T00:13:15Z

static/js/stream_edit.js

+        confirm_dialog.launch({
+            parent: modal_parent,
+            html_heading: $t_html(
+                {defaultMessage: "Archive stream {stream}"},


The stream name here seems to not have the styling it had previously.

Maybe similar comment here as the above one ?

timabbott · 2021-06-03T00:14:34Z

static/templates/settings/deactivate_realm_modal.hbs

-    <div class="modal-header">
-        <button type="button" class="close" data-dismiss="modal" aria-label="{{t 'Close' }}"><span aria-hidden="true">&times;</span></button>
-        <h3 id="deactivate_realm_modal_label">
-            {{t "Deactivate organization" }} <span class="realm_name"></span>


Did this realm_name spam display the realm name before this change?

I'm not sure we need it here (we definitely want it in the modal somewhere), but we shouldn't be accidentally changing it in this commit.

No, the realm_name span didn't actually contain the realm name.
It's just a empty span added to the modal heading. (Not sure on why was it exactly added!)

Here's the relevant screenshot of browser inspector:

OK, this would have been a key detail to explain in the commit message.

Can you start a #design thread with that screenshot about where we should put the identity of the organization to deactivate? I do think we probably want to display it somewhere in that modal.

timabbott · 2021-06-03T00:20:12Z

I merged the first several commits as the series ending with 68111d9. Thanks @aryanshridhar! There may be a second pass of work worth doing of reading the code surrounding these for things that don't make sense; e.g. with the revoke_invite_modal, we seem to have duplicate ways of storing which things we're in the process of trying to revoke, the meta fields and the .confirm_dialog_yes_button attributes.

I'd expect us to just store those values in the closure rather in the DOM, i.e. the on_click function can call do_remoke_invite(invite_id, is_multiuse) without either of these storage methods.

aryanshridhar · 2021-06-06T17:46:22Z

Cool! Working on it

aryanshridhar · 2021-06-08T18:57:48Z

but am just not sure how these parent elements are/were decided.

@sahil839, I decided these parent elements by using browser inspector and figuring out the parent element for the relevant modal.

aryanshridhar · 2021-06-08T18:59:08Z

Can you review it again @timabbott ?

…dule. The "email" span in the old modal was not used for styling, just to support updating the field visually.

…ule. The stream_name CSS class in the heading before this change had no functional effect.

…ule. The realm_name field in the old modal was always empty.

timabbott · 2021-06-09T01:05:10Z

Merged, after extending the commit messages to explain that the CSS class spans that were removed were not functional. Thanks @aryanshridhar!

I think we still have a couple modals that can be migrated:

templates/zerver/app/delete_message.html (Covered in refactor: Migrate Bootstrap modal calls to overlay calls. #18304)
templates/zerver/app/deprecation_notice.html => Should be moved to a frontend template feature_deprecation_notice.hbs.

I'll start a #frontend thread about this soon, but we have a goal of moving everything out of templates/zerver/app.

I think it could also be a good followup to move all the confirm_dialog templates to static/templates/confirm_dialog/, similar to the static/templates/navbar_alerts/ directory we recently added, just from a codebase organization standpoint. Want to work on that next?

zulipbot added the size: XL label May 27, 2021

aryanshridhar force-pushed the Add-ConfirmDialog branch from 82b300a to 4cec136 Compare May 27, 2021 18:58

timabbott reviewed May 27, 2021

View reviewed changes

aryanshridhar force-pushed the Add-ConfirmDialog branch 3 times, most recently from 909283a to e5b6ed8 Compare May 28, 2021 17:29

sahil839 reviewed May 31, 2021

View reviewed changes

timabbott reviewed Jun 3, 2021

View reviewed changes

aryanshridhar mentioned this pull request Jun 3, 2021

refactor: Migrate Bootstrap modal calls to overlay calls. #18304

Merged

aryanshridhar force-pushed the Add-ConfirmDialog branch from e5b6ed8 to 1d93ac2 Compare June 8, 2021 18:26

aryanshridhar added 3 commits June 8, 2021 17:57

settings_users: Refactor user_deactivation modal to confirm_dialog mo…

418c40b

…dule. The "email" span in the old modal was not used for styling, just to support updating the field visually.

stream_edit: Refactor stream_deactivation modal to confirm_dialog mod…

ef17d02

…ule. The stream_name CSS class in the heading before this change had no functional effect.

settings_org: Refactor realm_deactivation modal to confirm_dialog mod…

281c7da

…ule. The realm_name field in the old modal was always empty.

timabbott force-pushed the Add-ConfirmDialog branch from 1d93ac2 to 281c7da Compare June 9, 2021 01:00

timabbott merged commit 281c7da into zulip:master Jun 9, 2021

aryanshridhar deleted the Add-ConfirmDialog branch June 9, 2021 10:52

aryanshridhar mentioned this pull request Jun 10, 2021

Move confirm_dialogs modals to a separate folder. #18780

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor certain modals to `confirm_dialog` module #18629

Refactor certain modals to `confirm_dialog` module #18629

aryanshridhar commented May 27, 2021 •

edited

timabbott May 27, 2021

timabbott May 27, 2021

aryanshridhar May 28, 2021

aryanshridhar May 28, 2021

timabbott May 27, 2021

timabbott commented May 27, 2021

aryanshridhar commented May 28, 2021

timabbott commented May 28, 2021

timabbott commented May 28, 2021

sahil839 May 31, 2021

aryanshridhar Jun 8, 2021

sahil839 May 31, 2021

sahil839 commented May 31, 2021

aryanshridhar commented May 31, 2021 •

edited

sahil839 commented Jun 1, 2021

timabbott commented Jun 3, 2021

timabbott Jun 3, 2021

aryanshridhar Jun 8, 2021 •

edited

aryanshridhar Jun 8, 2021

timabbott Jun 9, 2021

timabbott Jun 3, 2021

aryanshridhar Jun 8, 2021

timabbott Jun 3, 2021

aryanshridhar Jun 8, 2021 •

edited

timabbott Jun 9, 2021

timabbott commented Jun 3, 2021

aryanshridhar commented Jun 6, 2021

aryanshridhar commented Jun 8, 2021

aryanshridhar commented Jun 8, 2021

timabbott commented Jun 9, 2021

Refactor certain modals to confirm_dialog module #18629

Refactor certain modals to confirm_dialog module #18629

Conversation

aryanshridhar commented May 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented May 27, 2021

aryanshridhar commented May 28, 2021

timabbott commented May 28, 2021

timabbott commented May 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil839 commented May 31, 2021

aryanshridhar commented May 31, 2021 • edited

sahil839 commented Jun 1, 2021

timabbott commented Jun 3, 2021

Choose a reason for hiding this comment

aryanshridhar Jun 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aryanshridhar Jun 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Jun 3, 2021

aryanshridhar commented Jun 6, 2021

aryanshridhar commented Jun 8, 2021

aryanshridhar commented Jun 8, 2021

timabbott commented Jun 9, 2021

Refactor certain modals to `confirm_dialog` module #18629

Refactor certain modals to `confirm_dialog` module #18629

aryanshridhar commented May 27, 2021 •

edited

aryanshridhar commented May 31, 2021 •

edited

aryanshridhar Jun 8, 2021 •

edited

aryanshridhar Jun 8, 2021 •

edited