settings_org: Use settings_checkbox in Authentication Methods UI. #22369

alex-ter · 2022-07-03T21:02:57Z

Per review feedback in #21002 (direct link to the comment), replace HTML table with a series of settings_checkbox components for Authentication Methods UI.

Fixes #21001.

Notable points:

The way settings_checkbox is currently implemented, its instances for respective Auth Methods all get the same id, name and class. That doesn't seem to break anything (both automated and my manual testing), as those aren't used anywhere, but I'm open to suggestions if that's a problem.
I've removed the previous table-specific CSS entry and haven't added anything for the <div> I've introduced instead, as it doesn't seem to be necessary - but I'm open to other ideas, too.
I've aimed at minimum necessary changes, but deemed it meaningful to change a couple of related JS function names to use "list" instead of "table" to avoid confusion for those looking at the code in the future. At the same time I didn't change mentions of "row" as that would've been a bigger change and IMHO that still fits the "list" abstraction we now use here.
No test changes were necessary, only minor tweaking of the setting save/restore-related JS code

Screenshots and screen captures:
Before (logged in as Owner)

After (logged in as owner)

Self-review checklist

Self-reviewed the changes for clarity and maintainability
(variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

Explains differences from previous plans (e.g., issue description).
Highlights technical choices and bugs encountered.
Calls out remaining decisions and concerns.
Automated tests verify logic where appropriate (no changes in tests, run manual and automated tests before submitting the PR)

Individual commits are ready for review (see commit discipline).

Each commit is a coherent idea.
Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

Visual appearance of the changes.
Responsiveness and internationalization.
Strings and tooltips.
End-to-end functionality of buttons, interactions and flows.
Corner cases, error conditions, and easily imagined bugs.

Per review feedback in zulip#21002, replace HTML table with a series of settings_checkbox components for Authentication Methods UI. Also adjust relevant JS function naming accordingly, to avoid confusion. Fixes zulip#21001.

zulipbot · 2022-07-03T21:03:04Z

Hello @zulip/server-settings members, this pull request was labeled with the "area: settings (admin/org)" label, so you may want to check it out!

sov-j · 2022-07-04T16:02:02Z

It works as it was claimed. I have tested this on current Firefox, Firefox Dev and Chrome.

alex-ter · 2022-07-05T07:30:01Z

Thanks @sov-j!

Just to make it clear for folks looking at it - this PR is ready for review.

sahil839

Thanks for working on this @alex-ter. Posted one inline comment.
We should keep ids unique. We can add method name to the id I think.

And also I think currently the vertical space between two options is too much especially when the label is not too long. But I am not sure. Will wait for feedback from others on this.

sahil839 · 2022-07-05T17:48:49Z

static/js/settings_org.js

@@ -418,7 +418,7 @@ export function populate_auth_methods(auth_methods) {
    if (!meta.loaded) {
        return;
    }
-    const $auth_methods_table = $("#id_realm_authentication_methods").expectOne();
+    const $auth_methods_list = $("#id_realm_authentication_methods").expectOne();


I think it would be better if these variable name changes are done in a separate commit.

My underlying idea here was that those variable/function name changes are directly related to that component change, therefore making a logically whole one. Don't you think they would be more confusing when standing alone? I can change that no problem, just to make sure :)

timabbott · 2022-07-06T00:58:46Z

Yeah, I think I agree we probably want some special CSS for this page to reduce the spacing between options to make the visuals look a bit nicer. (It's possible a better solution would be extending the option labels to have a bit more detail about what these authentication methods are). So put that CSS in its own commit, so it's easy to drop/revert if we change our minds.

alex-ter · 2022-07-06T17:59:41Z

Thanks Sahil, Tim - appreciate your feedback!

@sahil839

We should keep ids unique. We can add method name to the id I think.

Yeah, I've considered a couple of approaches, and in the simplest case of minimum changes, we'd need to use the label var also in the id attribute value construction within that partial, but that will affect other instantiations. Do you think that would be appropriate? Or maybe there's a better way I'm overlooking here?

As for the CSS - ack, will add one in a separate commit.

zulipbot added size: L area: settings (admin/org) good first issue labels Jul 3, 2022

sahil839 reviewed Jul 5, 2022

View changes

settings_org: Use settings_checkbox in Authentication Methods UI. #22369

settings_org: Use settings_checkbox in Authentication Methods UI. #22369

alex-ter commented Jul 3, 2022

zulipbot commented Jul 3, 2022

sov-j commented Jul 4, 2022 •

edited

alex-ter commented Jul 5, 2022 •

edited

sahil839 left a comment

sahil839 Jul 5, 2022

alex-ter Jul 6, 2022

timabbott commented Jul 6, 2022 •

edited

alex-ter commented Jul 6, 2022

settings_org: Use settings_checkbox in Authentication Methods UI. #22369

Are you sure you want to change the base?

settings_org: Use settings_checkbox in Authentication Methods UI. #22369

Conversation

alex-ter commented Jul 3, 2022

zulipbot commented Jul 3, 2022

sov-j commented Jul 4, 2022 • edited

alex-ter commented Jul 5, 2022 • edited

sahil839 left a comment

sahil839 Jul 5, 2022

Choose a reason for hiding this comment

alex-ter Jul 6, 2022

Choose a reason for hiding this comment

timabbott commented Jul 6, 2022 • edited

alex-ter commented Jul 6, 2022

sov-j commented Jul 4, 2022 •

edited

alex-ter commented Jul 5, 2022 •

edited

timabbott commented Jul 6, 2022 •

edited