zulip icon indicating copy to clipboard operation
zulip copied to clipboard

Add "Alphabetize choices" button for "List of options" type custom profile fields.

Open tnmkr opened this issue 1 year ago • 10 comments

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
image image
image image
Enabled Disabled
image image
image image
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.

tnmkr avatar May 31 '24 04:05 tnmkr

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!

zulipbot avatar May 31 '24 04:05 zulipbot

@sanchi-t This PR is ready for review. Please take a look when possible :)

tnmkr avatar Jun 04 '24 17:06 tnmkr

@zulipbot label "buddy review"

tnmkr avatar Jun 04 '24 17:06 tnmkr

@tnmkr This PR ready for next round of reviews.

sanchi-t avatar Jun 22 '24 09:06 sanchi-t

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.

tnmkr avatar Jun 23 '24 11:06 tnmkr

@m-e-l-u-h-a-n Marking for review from you.

@zulipbot remove "buddy review" @zulipbot add "mentor review"

tnmkr avatar Jun 23 '24 11:06 tnmkr

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"

tnmkr avatar Jun 24 '24 03:06 tnmkr

@zulipbot add "maintainer review"

tnmkr avatar Jun 24 '24 03:06 tnmkr

I posted some thoughts on the #design thread.

alya avatar Jun 24 '24 22:06 alya

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.strcmp as it is more performant and used in other places.

tnmkr avatar Jun 30 '24 10:06 tnmkr

Works in my manual testing! @sahil839 are you up for doing a review pass on this one?

alya avatar Jul 30 '24 20:07 alya

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.

sahil839 avatar Jul 31 '24 12:07 sahil839

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.

sahil839 avatar Jul 31 '24 12:07 sahil839

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.

zulipbot avatar Aug 20 '24 17:08 zulipbot

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.

zulipbot avatar Aug 20 '24 17:08 zulipbot

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.

shuklamaneesh23 avatar Oct 09 '24 04:10 shuklamaneesh23

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.

sahil839 avatar Oct 10 '24 06:10 sahil839

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.

shuklamaneesh23 avatar Oct 10 '24 06:10 shuklamaneesh23

I cannot work on this for at least another week.

Feel free to create a PR @shuklamaneesh23, I'd be happy to review.

tnmkr avatar Oct 10 '24 10:10 tnmkr

Hello @sahil839 and @tnmkr , I have filed a PR #32660 fixing this. Please have a look at it. Thanks!

shuklamaneesh23 avatar Dec 10 '24 23:12 shuklamaneesh23