zulip
zulip copied to clipboard
Add "Alphabetize choices" button for "List of options" type custom profile fields.
Commit 1 Fixes a bug reported in CZO.
Commit 2 Fixes: #28607 CZO #design
Changes from previous PR #28867
- We disable the button instead of hiding it, this was decided in CZO thread linked in the issue.
- I do not fully understand the approach used in that PR and I think it's better to build this feature around
SortableJS.sort(), so I started from scratch in this PR.
Screenshots and screen captures:
| Before | After |
|---|---|
| Enabled | Disabled |
|---|---|
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).
- [ ] 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).
- [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 members, this pull request was labeled with the "area: settings (admin/org)" label, so you may want to check it out!
@sanchi-t This PR is ready for review. Please take a look when possible :)
@zulipbot label "buddy review"
@tnmkr This PR ready for next round of reviews.
I left some comments. Also, during manual testing, choices with spaces as prefixes cause entries like " z" to appear before "a" due to the leading space during sorting. We should probably discuss with CZO what the expected behavior should be. One consideration is whether we should strip spaces before sorting.
I think we want to alphabetize without whitespace, so I've added a follow up commit which should be squashed when merging. Thanks for flagging, highlighting to get opinions from others.
@m-e-l-u-h-a-n Marking for review from you.
@zulipbot remove "buddy review" @zulipbot add "mentor review"
I posted these concerns for feedback in original CZO thread.
@alya This needs some design decisions. Marking for maintainer review.
@zulipbot remove "mentor review" @zulipbot add "maintainer review"
@zulipbot add "maintainer review"
I posted some thoughts on the #design thread.
Updated with the following changes:
- Removed follow-up commit to trim whitespaces. (from #design)
- No longer disable the button. (from #design)
- Move empty choices to last when sorting (from #design)
- Use
utils.strcmpas it is more performant and used in other places.
Works in my manual testing! @sahil839 are you up for doing a review pass on this one?
Looks good to me. I am not sure about using delete button visibility to check whether to add a new row or not, but cannot think of a better solution which handles all cases, so it is probably fine for now.
One way can be to check if any input is empty among the list of inputs, but need to check how it would behave if user empties one of the existing option input.
Heads up @tnmkr, 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.
Heads up @tnmkr, 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.
One way can be to check if any input is empty among the list of inputs, but need to check how it would behave if user empties one of the existing option input.
Hey @sahil839, if you have any suggestions on how to proceed, I'd be happy to take this PR to completion since it seems inactive for a while.
This is mostly fine, except there might be a better way to check delete button visibility as I mentioned above. Would be good to confirm with @tnmkr about his availability once before working on this, since it might be quick for him to complete it than someone else.
Hey @tnmkr, are you available to work on completing this PR? If not, could you share your thoughts on how you'd proceed with the solution? I'd be happy to take it from there and finish it up.
I cannot work on this for at least another week.
Feel free to create a PR @shuklamaneesh23, I'd be happy to review.
Hello @sahil839 and @tnmkr , I have filed a PR #32660 fixing this. Please have a look at it. Thanks!