Closed JesusHdez960717 closed 3 months ago
@ueman have you been able to review this PR?
Unfortunately, it's not this easy to fix the issue. If you try running the example, click on the 'toggle feedback mode' button, and then when you use the drop down in the feedback view, it's broken, since there's no navigator anymore.
Okey thanks, let me review it and see if I can figure out anything else to fix it
Hi @ueman , I just submit another commit with the fix for the 'dropdown need navigator ancestor'
Solution: I wrap the feedback-builder with a navigation and it worked.
Description: From one side, if fixes the error when something in this builder need a Navigation as ancestor, and for the other side, because this builder only cover the bottom part of the overlay and not the hole app, it does not affect the navigation.
BTW, the other solution I found was to do the same thing from the 'developer' side, and wrap the feedback-builder with the navigation, and work as same, the difference is that it give the responsibility to the developer and should be an explicit explanation in this package docs to avoid future errors
Can you open a dialog from the custom feedback builder?
Solution: I wrap the feedback-builder with a navigation and it worked.
That's awesome. Can you also extend the example or test with a dialog, so that this behavior doesn't regress in the future?
BTW, the other solution I found was to do the same thing from the 'developer' side, and wrap the feedback-builder with the navigation, and work as same, the difference is that it give the responsibility to the developer and should be an explicit explanation in this package docs to avoid future errors
I don't like this solution. The developer shouldn't have to figure this out on his own, it should just work. It's the expected behavior from a consumer's point of view, so not enabling that seems like a bad design.
Attention: Patch coverage is 76.74419%
with 20 lines
in your changes are missing coverage. Please review.
Project coverage is 83.43%. Comparing base (
471cc7b
) to head (e9b1734
). Report is 10 commits behind head on master.
Files | Patch % | Lines |
---|---|---|
feedback/lib/src/feedback_widget.dart | 80.26% | 15 Missing :warning: |
feedback/lib/src/feedback_bottom_sheet.dart | 50.00% | 5 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Updated the example.
You can successfully open a dialog from the custom-feedback, the problem
is that because it's in a different navigator it get show only in the bottom part of app (and I don't know if that is the intended behavior)
I attached images of how it would look:
Basic Image with nothing open:
Image with both dialogs open, one from base scaffold and one from custom-feedback:
With the current code, the dialog would be shown properly, and not just in the bottom sheet. It's a very tricky problem :/
Maybe @caseycrogers has some ideas on how to fix the navigator issue?
At this point I consider that maybe we must put the options on the table and sacrifice something, the options I see are:
1 - Leave the plugin as is with the impossibility of using the dialogs if you use this package (because they dont pop) 2 - Update it with the changes that allow the normal use of navigation and dialogs poping correctly, but the behavior of the dialogs within the custom feedback builder are only shown at the bottom
I'm happy to dig into this this weekend and see if I can come up with a fix. Flutter's navigation handling for the Android back button is a dumpster fire, but it's a dumpster fire I've dove into a couple times before so there's a good chance to make everyone happy here.
All that said, showing a dialogue within a feedback flow and then having it properly respond to the back gesture is such a niche within a niche as you said it may also be reasonable to just pick one compromise or the other.
@ueman @caseycrogers any update guys?
I'll plan to merge this, but I'm currently busy 😅 Thank you very much for this tho!
Thanks to you, for me it is a pleasure be able to contribute to a package as awesome as this one
Thanks @ueman for the approvement
:scroll: Description
When a dialog is showed, and the back button in Android is pressed, the dialog don't pop, instead the
back
action is executen in the background screenThe error was given because the use of an
navigation
widget in the widget tree prevent the back action from actually poping the dialog (btw, I'm not 100% sure why this bug is reproduced, but I know that with this change it get solved)Before:
After:
:bulb: Motivation and Context
This PR solve the
Dialog not popping
issue This issue is reported in::green_heart: How did you test it?
To tested, run the next code in the latest version of the plugin:
:pencil: Checklist
:crystal_ball: Next steps