Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add confirm dialog to "Convert to static list" #2392

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

ziggabyte
Copy link
Contributor

Description

This PR adds a confirm dialog with some info that highlights that converting from a dynamic to a static list will delete the Smart Search, which means the list will be emptied of people

Screenshots

bild

Changes

  • Adds confirm dialog with info when selecting "Convert to static list"

Notes to reviewer

no specific ones

Related issues

Resolves #532

@richardolsson
Copy link
Member

I didn't see this until just now. Nice work tackling this age old issue!

The issues describes two alternative solutions, to change the label in the menu and/or to add a confirmation dialog. To me, changing the label would have been the simpler option, although to be fair I'm not entirely sure what a better label would be. I'm curious, if you don't mind, to hear why you preferred the confirmation dialog over changing the label (or both).

@ziggabyte
Copy link
Contributor Author

I didn't see this until just now. Nice work tackling this age old issue!

The issues describes two alternative solutions, to change the label in the menu and/or to add a confirmation dialog. To me, changing the label would have been the simpler option, although to be fair I'm not entirely sure what a better label would be. I'm curious, if you don't mind, to hear why you preferred the confirmation dialog over changing the label (or both).

I tried to come up with a reasonable label that would convey that the Smart Search would be removed and the list become empty as a result of that, and I could not figure it out.
"Convert to static lists (removes everyone)" does not really do it for me, neither does "Remove Smart Search (convert to static list)", "Restart as static list". So I thought this was the easiest way to get the info in there, plus it feels like it's not an action that is performed so often that a confirm dialog would feel like a hill in the road.

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make a small change to the message so that it's not wrong in this one sense?

makeStatic: m('Convert to static list'),
makeStatic: {
confirmDialogInfo: m(
'If you convert this list to a static list the Smart Search will be removed and the list will be empty, as all the people the Smart Search returned will no longer be there.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The part about the "empty" list is not 100% correct. If there are statically added rows from before, those rows will still be there. Try doing this:

  1. Create a list
  2. Add a person
  3. Use ellipsis menu to convert to Smart Search list
  4. Configure Smart Search and save
  5. Use ellipsis menu again to convert to static list
  6. Read the message
  7. Confirm
  8. Contemplate message vs result 😊

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! 💯

@richardolsson richardolsson merged commit 1ba21e2 into main Dec 5, 2024
6 checks passed
@richardolsson richardolsson deleted the issue-532/convert-to-static-list-info branch December 5, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Label "Convert to static view" does not sufficiently communicates that view will be cleared
2 participants