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.

Sign up for GitHub

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

Jump to bottom

Resolving #32: implement a warning modal on deleting a file/folder while s… #60

Merged
merged 4 commits into from
Jul 4, 2020
Merged

Resolving #32: implement a warning modal on deleting a file/folder while s… #60

merged 4 commits into from
Jul 4, 2020

Conversation

mhdmhsni
Copy link
Contributor

@mhdmhsni mhdmhsni commented Jul 2, 2020

Hi @imsnif, this commit resolves #32.

  • added WarningBox to modals module.
  • rendering logic is similar to other modals. nothing specific to mention.
  • about the auto close functionality i added a WarningTimeout variant to your Event enum. this event waits for 5 seconds then resets the ui state using Instruction::ResetUiMode.

please let me know if anything needs to change.

feat(app) implement a warning modal on deleting a file/folder while s…
a9709d6 
…canning

add keypress handler for warning_modal

update apps ui state of warning modal

implement autoclose functionality for warningmodal
@mhdmhsni mhdmhsni changed the title Resolving https://github.com/imsnif/diskonaut/issues/32 feat(app) implement a warning modal on deleting a file/folder while s… Resolving #32: implement a warning modal on deleting a file/folder while s… Jul 2, 2020
@imsnif
Copy link
Owner

imsnif commented Jul 3, 2020

Hey @mhdmhsni, good job finding how to make this disappear with the Event mechanism. That's exactly the right way - sorry I wasn't around to make this easier. :)

I tested this branch and found a bug. While loading, if I try to delete a file, then dismiss the message by pressing ENTER, then try to delete the same file again quickly, everything gets stuck (might have to do this a few times fast).
I didn't investigate very deeply, but I suspect there is some sort of deadlock between the instruction channel and the park timeout.
While we can try and work around this, I feel the best way would be to give up on the timeout and wait for the user to dismiss this manually. I feel this is not bad UX and will save us some headaches. What do you think?

@mhdmhsni
Copy link
Contributor Author

mhdmhsni commented Jul 3, 2020

Hey @imsnif. Oops! I didn't see that bug coming. What comes to my mind about this situation is that maybe this ‘WarningTimeout‘ event should be cancellable. I mean if we close the warning modal manually, the timeout event keeps going (but it has to be cancelled automatically).
But i personally think giving more options to user to do a single thing, means adding more and more possibility of unwanted and unpredictable bugs and issues. So I'm agree with you.
so if any change needs to be done on this branch, please let me know. Thanks.

@imsnif
Copy link
Owner

imsnif commented Jul 3, 2020

Sounds good. So do you want to remove the cancellation timeout and make it only dismissable through the user pressing any key?

@mhdmhsni
Copy link
Contributor Author

mhdmhsni commented Jul 3, 2020

yes. i think regarding to the current situation this is the best approach. but maybe we can investigate about the auto close problem in another issue or something.

@imsnif
Copy link
Owner

imsnif commented Jul 3, 2020

Hey @mhdmhsni - changes look good. I'm going to take a closer look tomorrow.

imsnif
imsnif approved these changes Jul 4, 2020
View reviewed changes
Copy link
Owner

@imsnif imsnif left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the work and the changes!
I changed the deletion key to DELETE, since we already changed that in main. :)

@imsnif imsnif merged commit 356904f into imsnif:main Jul 4, 2020
@mhdmhsni
Copy link
Contributor Author

mhdmhsni commented Jul 4, 2020

Sorry, i forgot to change it myself after merging main branch into this one. thanks a lot.

@mhdmhsni mhdmhsni deleted the issue32 branch July 4, 2020 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imsnif imsnif imsnif approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: when trying to delete something while scanning, show a descriptive error message
2 participants
@mhdmhsni @imsnif