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

feat: add close channel dialog #378

Merged
merged 5 commits into from
Aug 7, 2024
Merged

feat: add close channel dialog #378

merged 5 commits into from
Aug 7, 2024

Conversation

im-adithya
Copy link
Member

@im-adithya im-adithya commented Jul 31, 2024

Description

window.alert, prompt and confirm don't work in wails. We can use Dialog but it has some limitations in MacOS. Also the flow for closing a channel involves the user to type "force close" which is not ideal. This PR replaces those with AlertDialog from shadcn for Wails support as well as design consistency

Screenshot 2024-07-31 at 9 10 18 PM Screenshot 2024-07-31 at 9 10 30 PM Screenshot 2024-08-07 at 4 45 05 PM Screenshot 2024-08-07 at 4 45 20 PM Screenshot 2024-07-31 at 9 11 01 PM

Copy link
Contributor

@reneaaron reneaaron left a comment

Choose a reason for hiding this comment

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

Just from a quick glance at the screenshots:

  1. "Proceed only if you still want to continue" - Either remove or rephrase, but this feels wrong.

  2. If you present the user with 2 options (where the user can only pick one) rather use selects / radios than checkboxes, checkboxes always suggest you can pick multiple ones)

@im-adithya
Copy link
Member Author

"Proceed only if you still want to continue" - Either remove or rephrase, but this feels wrong.

Yeah felt weird to me as well, will remove it entirely, maybe we can change the "Continue" in the button text to something else?

rather use selects / radios than checkboxes

I was a bit hesitant to use Radio as that would add another component, so I used existing one. I didn't use Select/Boxes like budget options as I wanted to add description so it's more understandable for the user. So Radio?

@reneaaron
Copy link
Contributor

"Proceed only if you still want to continue" - Either remove or rephrase, but this feels wrong.

Yeah felt weird to me as well, will remove it entirely, maybe we can change the "Continue" in the button text to something else?

Sure, maybe "Confirm"?

rather use selects / radios than checkboxes

I was a bit hesitant to use Radio as that would add another component, so I used existing one. I didn't use Select/Boxes like budget options as I wanted to add description so it's more understandable for the user. So Radio?

Yeah, I'd go for that one. 👍

@im-adithya
Copy link
Member Author

Done!

@rolznz
Copy link
Contributor

rolznz commented Aug 6, 2024

@reneaaron @im-adithya I'm not sure about the UI. Ideally users do normal closures, so I don't think they should have to pick between the two like this. The force closure option should be a last resort

@im-adithya
Copy link
Member Author

We agreed on making the copy understandable and dealing with the Normal / Force Close UI later. Ready for review!

Copy link
Contributor

@rolznz rolznz left a comment

Choose a reason for hiding this comment

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

tACK

@rolznz rolznz merged commit 097436b into master Aug 7, 2024
8 checks passed
@rolznz rolznz deleted the task-close-channel branch August 7, 2024 13:40
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.

3 participants