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
[Material] Unit test for skipping Slider tick mark due to overdensity #28013
[Material] Unit test for skipping Slider tick mark due to overdensity #28013
Conversation
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
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
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.
Looks good, just some small stuff
|
||
Widget buildSlider({ | ||
int divisions, | ||
TextDirection textDirection, |
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.
This parameter isn't needed
max: 100.0, | ||
divisions: divisions, | ||
value: value, | ||
onChanged: (double newValue) { |
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.
Simplify for this test, since we're not moving the slider:
onChanged: (double value) { },
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.
Done
@@ -978,6 +978,63 @@ void main() { | |||
await tester.pumpAndSettle(); | |||
}); | |||
|
|||
testWidgets('Tick marks are skipped when they are too dense', (WidgetTester tester) async { | |||
double value = 25.0; |
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.
Not needed for this test
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.
Done
), | ||
); | ||
|
||
// No tick marks because they are too dense, just a thumb. |
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.
The failure mode we've seen has divisions = max - min (100). Would be best to duplicate that here to make sure we're not regressing a case that we've seen fail. We can assume that if 100 is too dense 500 will be too :-)
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.
Done and added comment
}) { | ||
return Directionality( | ||
textDirection: textDirection, | ||
child: StatefulBuilder( |
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.
Don't need a StatefulBuilder, since we don't actually need to maintain the slider's value.
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.
Done
…er-tick-density-fix
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
…th/flutter into slider-tick-density-fix
…th/flutter into slider-tick-density-fix
Adding a unit test for skipping over dense tick marks and updating the previous hot fix with better style.