Fix the confusing-zero case with NestedScrollView. #14133
Conversation
|
cc @collinjackson for review |
|
Fixes #10658 |
There was a problem hiding this comment.
This is a pretty big change, and it's hard for me to reason about the ways it might break, but it seems to behave intuitively in my testing. It exposes a lot of new API surface area for us to maintain, but I don't see a good way around that.
I wonder if it would make sense to have the NestedScrollView constructor adds the injectors/absorber automagically (maybe also other things like PageStorageKey and SafeArea), and then have a NestedScrollView.custom constructor that gives lower-level control over these advanced widgets if you don't like the default behavior. If you agree that this is a good idea, it could be done in a separate pull request.
There was a problem hiding this comment.
typo in the comment here
There was a problem hiding this comment.
nit: this file was mostly fitting into the 80 character limit before, consider breaking this line and the other long lines you added for readability
There was a problem hiding this comment.
Split most of the lines down. A few still exceed 80 chars but there's no clean way to split those.
There was a problem hiding this comment.
nit: consider breaking this line
There was a problem hiding this comment.
typo here ("sliver")
There was a problem hiding this comment.
I would probably break this line
There was a problem hiding this comment.
i left this one... it's part of a bunch of more or less identical methods that just duplicate their arguments into constructor calls. It's probably easier to read if it's just skipped over as obviously mechanical code.
There was a problem hiding this comment.
Cascade operator looks kind of weird here if you're only setting one thing.
There was a problem hiding this comment.
Yeah. This is done for consistence with all the other updateRenderObject methods in the framework.
There was a problem hiding this comment.
Cascade operator looks kind of weird here if you're only setting one thing.
|
Thanks for the review! Will land on green unless I hear otherwise. |
|
lgtm |
* Fix the confusing-zero case with NestedScrollView. * Update mock_canvas.dart * Update tabs_demo.dart * more tweaks
[email protected]:flutter/engine.git/compare/fdaa7cf12175...ee4c2a5 git log fdaa7cf..ee4c2a5 --first-parent --oneline 2019-12-05 [email protected] Roll src/third_party/skia 6344c2937997..0af32fdf5fea (12 commits) (flutter#14139) 2019-12-05 [email protected] Wire up Opacity on Fuchsia, round 2 (flutter#14024) 2019-12-05 [email protected] Disable fml_tests until they're fixed on Fuchsia (flutter#14137) 2019-12-05 [email protected] Started specifying the OS version for running the tests. (flutter#14094) 2019-12-04 [email protected] Roll src/third_party/skia ccca30aad770..6344c2937997 (13 commits) (flutter#14133) 2019-12-04 [email protected] Expanded our scenario_app docs. (flutter#14136) 2019-12-04 [email protected] [web][felt] fix source map path (flutter#14134) 2019-12-04 [email protected] Fix platform view offsets incorrectly taking into account device pixel ratios. (flutter#14135)
[email protected]:flutter/engine.git/compare/fdaa7cf12175...ee4c2a5 git log fdaa7cf..ee4c2a5 --first-parent --oneline 2019-12-05 [email protected] Roll src/third_party/skia 6344c2937997..0af32fdf5fea (12 commits) (#14139) 2019-12-05 [email protected] Wire up Opacity on Fuchsia, round 2 (#14024) 2019-12-05 [email protected] Disable fml_tests until they're fixed on Fuchsia (#14137) 2019-12-05 [email protected] Started specifying the OS version for running the tests. (#14094) 2019-12-04 [email protected] Roll src/third_party/skia ccca30aad770..6344c2937997 (13 commits) (#14133) 2019-12-04 [email protected] Expanded our scenario_app docs. (#14136) 2019-12-04 [email protected] [web][felt] fix source map path (#14134) 2019-12-04 [email protected] Fix platform view offsets incorrectly taking into account device pixel ratios. (#14135)
[email protected]:flutter/engine.git/compare/fdaa7cf12175...ee4c2a5 git log fdaa7cf..ee4c2a5 --first-parent --oneline 2019-12-05 [email protected] Roll src/third_party/skia 6344c2937997..0af32fdf5fea (12 commits) (#14139) 2019-12-05 [email protected] Wire up Opacity on Fuchsia, round 2 (#14024) 2019-12-05 [email protected] Disable fml_tests until they're fixed on Fuchsia (#14137) 2019-12-05 [email protected] Started specifying the OS version for running the tests. (#14094) 2019-12-04 [email protected] Roll src/third_party/skia ccca30aad770..6344c2937997 (13 commits) (#14133) 2019-12-04 [email protected] Expanded our scenario_app docs. (#14136) 2019-12-04 [email protected] [web][felt] fix source map path (#14134) 2019-12-04 [email protected] Fix platform view offsets incorrectly taking into account device pixel ratios. (#14135) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
3 participants
No description provided.