-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
All dialogs should have atleast one button #6076
Comments
@parneet-guraya Yes, I completely agree with you. It can be quite frustrating at times to have to reopen the app. |
@sonalyadav1 Big reason of making them modal was if user touched outside accidentally when dialog was shown, it led to user missing the dialog (You can see it in the PR's fixed issue). Hope that makes sense :-) P.S: Little tip for you, whenever trying to work on some part/feature/bug try to find related issue or why it was done the way it is, sometimes it is intended. So, this will also avoid unintentional rejections as well as lead you to write better solutions. For eg. the solution that you pushed fixed the problem no doubt, but if you did find that it was intentionally kept (non dismissable) you could have covered more such dialogs that were also needed the fix (same as I done in this PR #6078 . |
@parneet-guraya Thank you for the explanation and the helpful tip! I understand now why the dialogs were made non-dismissible and will ensure to check related issues and PRs more thoroughly in the future. I’ll review PR #6078 for reference and strive to align my solutions better with the project’s intent. Thanks again for the guidance! 😊 |
I realize I have been spelling |
No worries, it happens to the best of us! 😊 |
Good catch about the "report violation" popup! 🙂 |
In some dialogs there's neither positive nor negative button available since dialogs are modal (non dismissable) it blocks the app. So, solution is to keep the dialogs modal but add atleast cancel or dismiss button.
document_6294057105208906596.mp4
Do you want to work on this?
Yes
The text was updated successfully, but these errors were encountered: