settings: Replace extraneous pointer cursors on text with text cursor. #25071

palashb01 · 2023-04-11T05:33:36Z

Replace extraneous pointer cursors on text with text cursor, where the label is not associated with any input element.
Fix interaction between label and input element to focus on input/select fields on clicking on label.

Screenshots and screen captures:

Before	After

Comment for reviewer:

In the Personal Settings > Profile section, we have multiple input and select elements with a single label for all of them. With the new approach of fixing the for and id attributes, everything works fine and as expected, except for the birthday and mentor inputs. These inputs have a div element that doesn't show any interaction with the label.

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.

zulipbot · 2023-04-11T05:33:44Z

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

palashb01 · 2023-04-11T13:19:30Z

Hey @alya , I have opened a new pull request for #24623 after the review by @timabbott , with a new approach. Could you please review the pull request? Thanks :)

alya · 2023-04-13T05:01:22Z

Looks good to me in manual testing! @sahil839 could you please take a look?

sahil839

Posted a couple of comments.

A couple of points to confirm from @alya

We open the "Change email" and "Change password" modal on clicking the labels after changes in this PR. I don't have any strong opinion on this, but just wanted to point it out in case you missed it while testing. Also, we should probably mention about this change in the commit message as well.
Do we want a regular cursor or text cursor? By regular cursor, I mean the arrow which is shown by default.

sahil839 · 2023-04-13T06:36:05Z

web/templates/settings/custom_user_profile_field.hbs

@@ -1,5 +1,5 @@
 <div class="custom_user_field" name="{{ field.name }}" data-field-id="{{ field.id }}">
-    <label class="inline-block" for="{{ field.name }}" class="title">{{ field.name }}</label>
+    <label for="{{field.name}}" class="inline-block title">{{ field.name }}</label>


We can avoid removing the space before and after field.name in this commt.

sahil839 · 2023-04-13T06:56:38Z

web/templates/settings/organization_permissions_admin.hbs

-                <label for="realm_move_messages_within_stream_limit_seconds" class="dropdown-title">{{t "Time limit for editing topics" }} <i>({{t "does not apply to moderators and administrators" }})</i></label>
-                <select name="realm_move_messages_within_stream_limit_seconds" id="id_realm_move_messages_within_stream_limit_seconds" class="prop-element settings_select" data-setting-widget-type="time-limit">
+                <label for="id_realm_move_messages_within_stream_limit_seconds" class="dropdown-title">{{t "Time limit for editing topics" }} <i>({{t "does not apply to moderators and administrators" }})</i></label>
+                <select name="realm_move_messages_within_stream_limit_seconds" id="id_realm_move_messages_within_stream_limit_seconds" class="prop-element bootstrap-focus-style settings_select" data-setting-widget-type="time-limit">


bootstrap-focus-style can be added in a separate prep commit.

alya · 2023-04-18T19:49:08Z

Do we want a regular cursor or text cursor? By regular cursor, I mean the arrow which is shown by default.

Is this when things aren't clickable? Based on the CZO discussion, we decided to go with the browser default cursor.

@_Sahil Batra|10242 said:

The browser default (atleast for Chrome and Firefox) value of cursor property is default for label elements, which is typically an arrow but can be platform dependent.

alya · 2023-04-18T19:50:10Z

We open the "Change email" and "Change password" modal on clicking the labels after changes in this PR. I don't have any strong opinion on this, but just wanted to point it out in case you missed it while testing. Also, we should probably mention about this change in the commit message as well.

Ah, I hadn't noticed -- thanks for the callout!

I don't have a strong opinion either. Having clicking on the label open the modal seems fine, so let's just update the commit message.

palashb01 · 2023-04-21T13:38:26Z

Update: There has been a minor update in this PR after the discussion on the CZO, I am currently working on release goals PRs and will continue working on this after 4-5 days.

zulipbot · 2023-04-25T00:33:54Z

Heads up @palashb01, 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.

This commit changes the incorrect 'for' attribute values to the correct ones in order to fix the interaction between label, select/ button elements.

This commit adds or removes 'id' and 'for' attribute values to fix the interaction between label and input elements, so that clicking on the label will focus the corresponding input element. For labels that are not associated with any input element, the 'for' attribute has been removed. To fix the issue of extraneous pointer cursors on labels, a CSS rule has been added so that a default text cursor will be displayed when there is no 'for' attribute. Fixes: zulip#21769

palashb01 · 2023-04-25T21:58:25Z

@zulipbot remove "has conflicts"

zulipbot · 2023-05-12T18:18:22Z

Heads up @palashb01, 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.

zulipbot added size: XL area: settings (user) area: settings (admin/org) area: stream settings bug labels Apr 11, 2023

palashb01 mentioned this pull request Apr 11, 2023

settings: Replace extraneous pointer cursors on text with text cursor. #24623

Closed

14 tasks

sahil839 reviewed Apr 13, 2023

View reviewed changes

zulipbot added the has conflicts label Apr 25, 2023

palashb01 added 3 commits April 25, 2023 19:29

settings: Remove duplicate class attribute from label element.

ef05daf

settings: Refactor the correct for attribute values.

e8f91c2

This commit changes the incorrect 'for' attribute values to the correct ones in order to fix the interaction between label, select/ button elements.

palashb01 force-pushed the Issue--#21769 branch from 4e54f67 to dfaa25c Compare April 25, 2023 14:02

zulipbot removed the has conflicts label Apr 25, 2023

zulipbot added the has conflicts label May 12, 2023

timabbott force-pushed the main branch 2 times, most recently from 4ec3636 to 88b200c Compare August 18, 2023 23:51

timabbott added the completion candidate PRs with reviews that may unblock merging label Sep 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

settings: Replace extraneous pointer cursors on text with text cursor. #25071

settings: Replace extraneous pointer cursors on text with text cursor. #25071

palashb01 commented Apr 11, 2023 •

edited

zulipbot commented Apr 11, 2023

palashb01 commented Apr 11, 2023

alya commented Apr 13, 2023

sahil839 left a comment

sahil839 Apr 13, 2023

sahil839 Apr 13, 2023

alya commented Apr 18, 2023 •

edited

alya commented Apr 18, 2023

palashb01 commented Apr 21, 2023 •

edited

zulipbot commented Apr 25, 2023

palashb01 commented Apr 25, 2023

zulipbot commented May 12, 2023

settings: Replace extraneous pointer cursors on text with text cursor. #25071

Are you sure you want to change the base?

settings: Replace extraneous pointer cursors on text with text cursor. #25071

Conversation

palashb01 commented Apr 11, 2023 • edited

zulipbot commented Apr 11, 2023

palashb01 commented Apr 11, 2023

alya commented Apr 13, 2023

sahil839 left a comment

Choose a reason for hiding this comment

sahil839 Apr 13, 2023

Choose a reason for hiding this comment

sahil839 Apr 13, 2023

Choose a reason for hiding this comment

alya commented Apr 18, 2023 • edited

alya commented Apr 18, 2023

palashb01 commented Apr 21, 2023 • edited

zulipbot commented Apr 25, 2023

palashb01 commented Apr 25, 2023

zulipbot commented May 12, 2023

palashb01 commented Apr 11, 2023 •

edited

alya commented Apr 18, 2023 •

edited

palashb01 commented Apr 21, 2023 •

edited