-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix GlowingOverscrollIndicator examples
#155203
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
Fix GlowingOverscrollIndicator examples
#155203
Conversation
nate-thegrate
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and pushed the example updates to this branch.
We should be good to add tests that look for the GlowingOverscrollIndicator now!
GlowingOverscrollIndicator examples
|
Please let me know if these tests are sufficient. I encountered some unexpected behaviors during the test, so I decided not to test behaviors for now. Different occurrencesI tried: testWidgets('raw occurrences', (WidgetTester tester) async {
await tester.pumpWidget(const example.GlowingOverscrollIndicatorExampleApp());
final Finder goiFinder = find.byType(GlowingOverscrollIndicator);
print(goiFinder);
});and when testing glowing_overscroll_indicator.0.dart the output returns one widget which is expected, but when testing glowing_overscroll_indicator.1.dart the output returns two widgets which is a bit confusing to me. In the second case, the widget tree structure is: - GlowingOverscrollIdicatorExampleApp
- MaterialApp
- Scaffold
- GlowingOverscrollIdicatorExample
- NestedScrollView
- GlowingOverscrollIdicator
- SliverAppBar
- Text
- CustomScrollView
- GlowingOverscrollIdicator
- ...
- ...
First I thought it was a result of using NestedScrollView, but I guess it's not. Dragging and calculating the positionI tried to test both examples using So, I hope that the visibility tests will be sufficient. |
After looking it over again, I think that was my fault 😅 I'll go ahead and push the fix (and restore the tests you added previously); if everything passes then I think we'll be good to merge! |
TahaTesser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing!
examples/api/lib/widgets/overscroll_indicator/glowing_overscroll_indicator.0.dart
Outdated
Show resolved
Hide resolved
examples/api/test/widgets/overscroll_indicator/glowing_overscroll_indicator.1_test.dart
Outdated
Show resolved
Hide resolved
examples/api/test/widgets/overscroll_indicator/glowing_overscroll_indicator.1_test.dart
Outdated
Show resolved
Hide resolved
examples/api/test/widgets/overscroll_indicator/glowing_overscroll_indicator.0_test.dart
Outdated
Show resolved
Hide resolved
examples/api/test/widgets/overscroll_indicator/glowing_overscroll_indicator.0_test.dart
Outdated
Show resolved
Hide resolved
examples/api/lib/widgets/overscroll_indicator/glowing_overscroll_indicator.0.dart
Show resolved
Hide resolved
examples/api/lib/widgets/overscroll_indicator/glowing_overscroll_indicator.1.dart
Show resolved
Hide resolved
nate-thegrate
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TahaTesser thanks very much for the review! Since I contributed the example updates, I'll respond to those; I'll leave test updates to @miechoo :)
examples/api/lib/widgets/overscroll_indicator/glowing_overscroll_indicator.0.dart
Show resolved
Hide resolved
examples/api/lib/widgets/overscroll_indicator/glowing_overscroll_indicator.0.dart
Outdated
Show resolved
Hide resolved
examples/api/lib/widgets/overscroll_indicator/glowing_overscroll_indicator.1.dart
Show resolved
Hide resolved
|
Hi guys. Should we do something more with this PR? |
|
Generally we give code reviewers 2 weeks before pinging them, since they often have busy lives with many responsibilities. 🙂 |
examples/api/lib/widgets/overscroll_indicator/glowing_overscroll_indicator.0.dart
Show resolved
Hide resolved
examples/api/test/widgets/overscroll_indicator/glowing_overscroll_indicator.0_test.dart
Outdated
Show resolved
Hide resolved
examples/api/test/widgets/overscroll_indicator/glowing_overscroll_indicator.1_test.dart
Outdated
Show resolved
Hide resolved
e97d01d to
460bcf5
Compare
bleroux
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
TahaTesser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT with nit
examples/api/lib/widgets/overscroll_indicator/glowing_overscroll_indicator.0.dart
Outdated
Show resolved
Hide resolved
examples/api/lib/widgets/overscroll_indicator/glowing_overscroll_indicator.1.dart
Outdated
Show resolved
Hide resolved
…oll_indicator.0_test.dart Co-authored-by: Bruno Leroux <[email protected]>
Co-authored-by: Taha Tesser <[email protected]>
9d01435 to
5a5e70b
Compare
|
auto label is removed for flutter/flutter/155203, due to - The status or check suite Windows tool_integration_tests_2_9 has failed. Please fix the issues identified (or deflake) before re-applying this label. |
Manual roll requested by [email protected] flutter/flutter@fe71cad...0fe6153 2024-10-31 [email protected] Roll Flutter Engine from c40b0b602822 to f2154ef3e31c (8 revisions) (flutter/flutter#157926) 2024-10-31 [email protected] Hides added routes before top-most route finishes pushing (flutter/flutter#156104) 2024-10-31 [email protected] Fix cursor on hover expand/collapse icon (#155207) (flutter/flutter#155209) 2024-10-31 [email protected] Add test for `media_query_data.system_gesture_insets.0.dart` (flutter/flutter#157854) 2024-10-31 [email protected] Add and plumb `useImplicitPubspecResolution` across `flutter_tools`. (flutter/flutter#157879) 2024-10-31 [email protected] Roll Flutter Engine from 090c33aeae83 to c40b0b602822 (1 revision) (flutter/flutter#157896) 2024-10-31 [email protected] Roll Flutter Engine from 9295eeb6d3ce to 090c33aeae83 (4 revisions) (flutter/flutter#157893) 2024-10-30 [email protected] Roll Flutter Engine from 2bd854e23b61 to 9295eeb6d3ce (5 revisions) (flutter/flutter#157882) 2024-10-30 [email protected] Adds a new helpful tool exit message for SocketExceptions thrown during mdns discovery (flutter/flutter#157638) 2024-10-30 [email protected] Fix `GlowingOverscrollIndicator` examples (flutter/flutter#155203) 2024-10-30 [email protected] Roll Flutter Engine from 906a7ad88052 to 2bd854e23b61 (1 revision) (flutter/flutter#157878) 2024-10-30 [email protected] Upgrade templates to AGP 8.7/Gradle 8.10.2 (flutter/flutter#157872) 2024-10-30 [email protected] Roll Flutter Engine from 57ed5d338e7e to 906a7ad88052 (13 revisions) (flutter/flutter#157875) 2024-10-30 [email protected] Update Material 3 `LinearProgressIndicator` for new visual style (flutter/flutter#154817) 2024-10-30 [email protected] Roll Flutter Engine from 999797a2f690 to 57ed5d338e7e (5 revisions) (flutter/flutter#157833) 2024-10-30 [email protected] Add hidden `--no-implicit-pubspec-resolution` flag for one stable release. (flutter/flutter#157635) 2024-10-30 [email protected] Roll Packages from 028027e to 7cc1caa (5 revisions) (flutter/flutter#157864) 2024-10-30 [email protected] Mention partial PRs in the contributing docs (flutter/flutter#157863) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Part of #130459
This pull request adds tests for the example files shown in the GlowingOverscrollIndicator Flutter API reference documentation.