zulip icon indicating copy to clipboard operation
zulip copied to clipboard

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

Open palashb01 opened this issue 1 year ago • 9 comments

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

Fixes: #21769


Screenshots and screen captures:

Before After
before interaction
text-before text

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.

birthday


Self-review checklist
  • [x] Self-reviewed the changes for clarity and maintainability (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • [x] Explains differences from previous plans (e.g., issue description).
  • [x] Highlights technical choices and bugs encountered.
  • [x] Calls out remaining decisions and concerns.
  • [ ] Automated tests verify logic where appropriate.

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

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

Completed manual review and testing of the following:

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

palashb01 avatar Apr 11 '23 05:04 palashb01

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!

zulipbot avatar Apr 11 '23 05:04 zulipbot

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 :)

palashb01 avatar Apr 11 '23 13:04 palashb01

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

alya avatar Apr 13 '23 05:04 alya

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 avatar Apr 18 '23 19:04 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.

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.

alya avatar Apr 18 '23 19:04 alya

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.

palashb01 avatar Apr 21 '23 13:04 palashb01

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 avatar Apr 25 '23 00:04 zulipbot

@zulipbot remove "has conflicts"

palashb01 avatar Apr 25 '23 21:04 palashb01

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 avatar May 12 '23 18:05 zulipbot