settings: add tooltip to clarify invalid Jitsi URL #27594

peterssonlinn · 2023-11-07T12:05:19Z

Added tooltip to clearify why save button is disabled when invalid Jitsi URL.
Co-authored-by: Angelica Ferlin angelica.ferlin@gmail.com

Fixes #27511.

Screenshots and screen captures:
Gif belows shows the new tooltip.

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.

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.

alya · 2023-11-07T19:24:08Z

Thanks! However, you need to implement a tooltip that is exactly as specified in the issue description.

Also, please update your commit message to match the commit style guidelines?

peterssonlinn · 2023-11-07T19:55:08Z

Hi @alya!

So you mean that the tooltip should have the text "Cannot save invalid Jitsi server URL."?
I did not know how that would be when the page was translated, and because of that I used "Invalid URL". Thanks in advance!

alya · 2023-11-07T22:02:10Z

Yes, that's why that phrase was used in the issue. I don't see a problem with the translation.

If it was an intentional change, note that you checked off this item in the PR checklist without implementing it. The PR checklist is there for a good reason, as not following it wastes time and requires more review round-trips. ;)

Explains differences from previous plans (e.g., issue description).

peterssonlinn · 2023-11-08T13:56:34Z

Yes, that's why that phrase was used in the issue. I don't see a problem with the translation.

If it was an intentional change, note that you checked off this item in the PR checklist without implementing it. The PR checklist is there for a good reason, as not following it wastes time and requires more review round-trips. ;)

Explains differences from previous plans (e.g., issue description).

We have now solved the issue and updated the tests so it includes "Jitsi". Thank you for the tip about the checklist, will look more carefully in the future. It is now ready for review.

alya · 2023-11-08T18:56:34Z

Your PR is not ready for review, as you have not addressed half of my feedback:

Also, please update your commit message to match the commit style guidelines.

angelicaferlin · 2023-11-13T13:14:35Z

Your PR is not ready for review, as you have not addressed half of my feedback:

Also, please update your commit message to match the commit style guidelines.

Hi, so sorry we missed that part but now it should be updated according to the guidelines now :)

alya · 2023-11-14T05:56:27Z

Thanks!

@sayamsamal could you please review this one? I have not tested it.

sayamsamal · 2023-11-14T12:06:36Z

web/templates/settings/settings_save_discard_widget.hbs

@@ -1,7 +1,7 @@
 {{#unless show_only_indicator}}
 <div class="save-button-controls hide">
    <div class="inline-block subsection-changes-save">
-        <button class="save-discard-widget-button button primary save-button" data-status="save">
+        <button class="save-discard-widget-button button primary save-button" data-status="save" id="save-discard-widget-id">


Let's not add an extra ID when we can use the preexisting .save-button class for the same purpose,

sayamsamal · 2023-11-14T12:07:37Z

web/src/tippyjs.js

+        onShow(instance) {
+            const $elem = $(instance.reference);
+            if ($($elem).find("#save-discard-widget-id").is(":disabled")) {
+                const content = $t({defaultMessage: "Cannot save invalid Jitsi server URL"});


Since the issue description asks for a period . at the end of the line, let's include that.

sayamsamal · 2023-11-14T13:35:43Z

tools/lib/capitalization.py

@@ -161,14 +162,17 @@
 SPLIT_BOUNDARY = "?.!"  # Used to split string into sentences.
 SPLIT_BOUNDARY_REGEX = re.compile(rf"[{SPLIT_BOUNDARY}]")

+# Regexes which check capitalization in sentences.


This comment is repeated twice, please recheck your code for simple errors as such to make it easier for the reviewers.

Also, the only change that should have been part of this commit in the current file is the addition of Jitsi in the ignored phrases. If you want to do additional cleanup, please include it as an additional commit with a commit message that explains those changes.

sayamsamal · 2023-11-14T13:36:32Z

tools/lib/capitalization.py

+    # Checks if an upper case character exists
+    r"^[A-Z][a-z]+[\sa-z0-9]+[A-Z]",
    # after a lower case character when the first character is in upper case.


Both the comments are actually part of a single comment.

sayamsamal · 2023-11-14T13:46:34Z

Thank you @peterssonlinn and @angelicaferlin. I have left you some feedback which should be rather quick to fix and will make the code a bit cleaner. That being said, the PR does work as intended and you have approached it in the right way, so good job on that.

sayamsamal · 2023-11-14T13:52:52Z

Also please provide the Co-authored-by line at the end of the commit message, to accurately credit the contributor.

Commit title

Commit message

Co-authored-by: Angelica Ferlin <angelica.ferlin@gmail.com>

angelicaferlin · 2023-11-15T09:29:05Z

Thank you @peterssonlinn and @angelicaferlin. I have left you some feedback which should be rather quick to fix and will make the code a bit cleaner. That being said, the PR does work as intended and you have approached it in the right way, so good job on that.

Thank you for the feedback and the kind words! We have now updated the PR with your suggestions :)

peterssonlinn · 2023-12-12T13:57:07Z

Hi @timabbott and @alya, I and Angelica would really appreciate some answers to the question that we have posted regarding how we can proceed. Could you please answer and give us some guidance?

timabbott · 2023-12-12T16:41:12Z

@sahil839 can you review this one? I'm not at all caught up on it.

This commit moves the function initialize_disable_btn_hint_popover from stream_ui_updates.js to settings_components.js due to circular dependencies. Co-authored-by: Angelica Ferlin <angelica.ferlin@gmail.com>

sahil839 · 2023-12-18T09:53:48Z

web/src/settings_components.js

+        const $button_wrapper = $subsection_elem.find(".subsection-changes-save");
+        const is_tippy = $button_wrapper.get(0)._tippy;
+        if (disable_save_btn) {
+            if (!is_tippy) {


Do we need this is_tippy condition? I don't think we do this check at other places.

We added a clarifying comment that explains why we need this. In short, we need to check that there is not already a tippy in place since this function runs every time a letter is entered in the URL box.

sahil839 · 2023-12-18T09:56:42Z

web/src/settings_components.js

+                    $t({defaultMessage: "Cannot save invalid Jitsi server URL."}),
+                );
+                $button_wrapper.get(0)._tippy.setProps({
+                    placement: "top",


To do this in a better way, we can pass opts object to initialize_disable_btn_hint_popover and then update the code there to be something like -

const tippy_opts = { animation: false, hideOnClick: false, placement: "bottom", ...opts, };

We updated to your suggestion

sahil839

Thanks for updating the PR. Posted a couple of comments, looks good otherwise.

This commit adds a tooltip in organization settings, when the save button is disabled due to invalid Jitsi URL. Fixes zulip#27511. Co-authored-by: Angelica Ferlin <angelica.ferlin@gmail.com>

peterssonlinn · 2023-12-18T14:51:23Z

Thanks for updating the PR. Posted a couple of comments, looks good otherwise.

@sahil839 we have updated the PR according to your suggestions. Thanks for the fast feedback. Let us know if there is anything else that needs to be updated. :)

sahil839 · 2023-12-22T11:11:45Z

web/src/settings_components.js

@@ -681,22 +681,40 @@ function enable_or_disable_save_button($subsection_elem) {
        disable_save_btn = should_disable_save_button_for_time_limit_settings(time_limit_settings);
    } else if ($subsection_elem.attr("id") === "org-other-settings") {
        disable_save_btn = should_disable_save_button_for_jitsi_server_url_setting();
+        const $button_wrapper = $subsection_elem.find(".subsection-changes-save");
+        const is_tippy = $button_wrapper.get(0)._tippy;


A better variable name here might be tippy or tippy_instance since it is not a boolean and is actually a tippy instance.

Also, would be good to update this to be $button_wrapper[0]._tippy just so that we have it consistent with other places in the codebase.

Looks good otherwise. Will mark it ready for "integration review" once these changes are done

alya · 2024-01-05T06:06:13Z

@peterssonlinn Do you plan to address @sahil839 's feedback above, or should we take over and finish this PR?

zulipbot · 2024-02-15T18:37:03Z

Heads up @peterssonlinn, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

timabbott · 2024-04-03T19:48:43Z

Closing in favor of #29386, thanks for your effort on this @peterssonlinn and @angelicaferlin!

zulipbot added the size: S label Nov 7, 2023

peterssonlinn force-pushed the 27511 branch from 0624219 to 323c271 Compare November 7, 2023 16:19

peterssonlinn force-pushed the 27511 branch 2 times, most recently from 475ed46 to 26f6639 Compare November 8, 2023 09:30

peterssonlinn force-pushed the 27511 branch from 26f6639 to b4b2d79 Compare November 9, 2023 09:30

zulipbot added size: M and removed size: S labels Nov 9, 2023

alya added the maintainer review PR is ready for review by Zulip maintainers. label Nov 14, 2023

sayamsamal reviewed Nov 14, 2023

View reviewed changes

peterssonlinn force-pushed the 27511 branch from b4b2d79 to e67bb38 Compare November 15, 2023 09:27

zulipbot added size: S and removed size: M labels Nov 15, 2023

zulipbot added the has conflicts label Nov 17, 2023

peterssonlinn force-pushed the 27511 branch from e67bb38 to e839ed0 Compare November 20, 2023 08:41

zulipbot removed the has conflicts label Nov 20, 2023

peterssonlinn force-pushed the 27511 branch 2 times, most recently from 4896f80 to f4985ef Compare November 21, 2023 08:36

zulipbot added size: S and removed size: M labels Nov 29, 2023

peterssonlinn force-pushed the 27511 branch from 1f825ce to baf4ac6 Compare December 4, 2023 07:40

peterssonlinn force-pushed the 27511 branch 4 times, most recently from 297e48d to 601e5ae Compare December 17, 2023 15:45

zulipbot added size: L has conflicts and removed size: S labels Dec 17, 2023

peterssonlinn force-pushed the 27511 branch from 601e5ae to a031613 Compare December 17, 2023 18:00

settings: Move init disable button function.

57c7781

This commit moves the function initialize_disable_btn_hint_popover from stream_ui_updates.js to settings_components.js due to circular dependencies. Co-authored-by: Angelica Ferlin <angelica.ferlin@gmail.com>

peterssonlinn force-pushed the 27511 branch from a031613 to e328d00 Compare December 18, 2023 09:26

zulipbot removed the has conflicts label Dec 18, 2023

sahil839 reviewed Dec 18, 2023

View reviewed changes

settings: Add tooltip to clarify invalid Jitsi URL.

0216363

This commit adds a tooltip in organization settings, when the save button is disabled due to invalid Jitsi URL. Fixes zulip#27511. Co-authored-by: Angelica Ferlin <angelica.ferlin@gmail.com>

peterssonlinn force-pushed the 27511 branch from e328d00 to 0216363 Compare December 18, 2023 14:49

sahil839 reviewed Dec 22, 2023

View reviewed changes

timabbott added the completion candidate PRs with reviews that may unblock merging label Feb 13, 2024

zulipbot added the has conflicts label Feb 15, 2024

kuv2707 mentioned this pull request Mar 21, 2024

settings: add tooltip to clarify invalid Jitsi URL - completion #29386

Merged

12 tasks

timabbott closed this Apr 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

settings: add tooltip to clarify invalid Jitsi URL #27594

settings: add tooltip to clarify invalid Jitsi URL #27594

peterssonlinn commented Nov 7, 2023 •

edited

alya commented Nov 7, 2023

peterssonlinn commented Nov 7, 2023

alya commented Nov 7, 2023

peterssonlinn commented Nov 8, 2023

alya commented Nov 8, 2023

angelicaferlin commented Nov 13, 2023

alya commented Nov 14, 2023

sayamsamal Nov 14, 2023

sayamsamal Nov 14, 2023

sayamsamal Nov 14, 2023

sayamsamal Nov 14, 2023

sayamsamal Nov 14, 2023

sayamsamal commented Nov 14, 2023

sayamsamal commented Nov 14, 2023

angelicaferlin commented Nov 15, 2023

peterssonlinn commented Dec 12, 2023

timabbott commented Dec 12, 2023

sahil839 Dec 18, 2023

peterssonlinn Dec 18, 2023

sahil839 Dec 18, 2023

peterssonlinn Dec 18, 2023

sahil839 left a comment

peterssonlinn commented Dec 18, 2023

sahil839 Dec 22, 2023

sahil839 Dec 22, 2023

alya commented Jan 5, 2024

zulipbot commented Feb 15, 2024

timabbott commented Apr 3, 2024

settings: add tooltip to clarify invalid Jitsi URL #27594

settings: add tooltip to clarify invalid Jitsi URL #27594

Conversation

peterssonlinn commented Nov 7, 2023 • edited

alya commented Nov 7, 2023

peterssonlinn commented Nov 7, 2023

alya commented Nov 7, 2023

peterssonlinn commented Nov 8, 2023

alya commented Nov 8, 2023

angelicaferlin commented Nov 13, 2023

alya commented Nov 14, 2023

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

sayamsamal commented Nov 14, 2023

sayamsamal commented Nov 14, 2023

angelicaferlin commented Nov 15, 2023

peterssonlinn commented Dec 12, 2023

timabbott commented Dec 12, 2023

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

sahil839 left a comment

Choose a reason for hiding this comment

peterssonlinn commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alya commented Jan 5, 2024

zulipbot commented Feb 15, 2024

timabbott commented Apr 3, 2024

peterssonlinn commented Nov 7, 2023 •

edited