zulip
zulip copied to clipboard
settings: Replace extraneous pointer cursors on text with text cursor.
- [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 |
---|---|
![]() |
![]() |
![]() |
![]() |
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
andid
attributes, everything works fine and as expected, except for thebirthday
andmentor
inputs. These inputs have a div element that doesn't show any interaction with the label.
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.
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!
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 :)
Looks good to me in manual testing! @sahil839 could you please take a look?
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 isdefault
forlabel
elements, which is typically an arrow but can be platform dependent.
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.
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.
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 remove "has conflicts"
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.