Skip to content

[Material] Create a themable Range Slider (continuous and discrete) #31681

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.

Sign up for GitHub

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

Jump to bottom
Merged
clocksmith merged 8 commits into from
Jun 6, 2019
Merged

[Material] Create a themable Range Slider (continuous and discrete) #31681

clocksmith merged 8 commits into from
Jun 6, 2019

Conversation

@clocksmith
Copy link
Contributor

@clocksmith clocksmith commented Apr 26, 2019 •
edited
Loading

Description

Creates a Material Design range slider.

The range slider is based off the updated slider, with the main difference being that it has 2 thumbs.
By default, the thumbs cannot pass each other and both the thumb and the value indicator create a stroke when they are overlapping for better visibility.

one
two
range_slider_disabled

Related Issues

#18083

Tests

I added the following tests:

Behavior tests for range slider have been added in range_slider_test.dart. Appearence tests will be updated with goldens.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@clocksmith clocksmith self-assigned this Apr 26, 2019
@goderbauer goderbauer added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Apr 29, 2019
rami-a
rami-a reviewed Apr 29, 2019
View reviewed changes
Copy link
Contributor

@rami-a rami-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments from reviewing range_slider.dart only so far. Will continue to review the rest of the change.

johnsonmh
johnsonmh reviewed Apr 29, 2019
View reviewed changes
packages/flutter/lib/src/material/slider_theme.dart Outdated
Copy link
Contributor

@johnsonmh johnsonmh Apr 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we try to stick to theming variables being overridable on the individual widgets, should we consider adding Color thumbStrokeColor as a field on this class, then doing thumbStrokeColor ?? sliderTheme.thumbStrokeColor here?

Copy link
Contributor Author

@clocksmith clocksmith Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slider is a bit different on the way it handles this. This slider theme these painters get is actually a copy where it already resolved any widget specific inputs (see line 511 in range_slider.dart)

rami-a
rami-a reviewed Apr 30, 2019
View reviewed changes
Copy link
Contributor

@rami-a rami-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a review of slider_theme.dart

@googlebot
Copy link

googlebot commented Apr 30, 2019

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

googlebot commented Apr 30, 2019

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

rami-a
rami-a reviewed Apr 30, 2019
View reviewed changes
Copy link
Contributor

@rami-a rami-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of test file

packages/flutter/test/material/range_slider_test.dart Outdated
Copy link
Contributor

@rami-a rami-a Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may be beneficial to avoid duplication and extract a buildApp function

Copy link
Contributor Author

@clocksmith clocksmith Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was done for all of the tests that did not need values, but there is not much value in doing it for the test that depend on setting the state of values (no pun intended)

packages/flutter/test/material/range_slider_test.dart Outdated
Copy link
Contributor

@rami-a rami-a Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is quite large, consider splitting it into smaller tests.

Copy link
Contributor Author

@clocksmith clocksmith Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I was able to split this up quite a bit.

johnsonmh
johnsonmh reviewed Apr 30, 2019
View reviewed changes
johnsonmh
johnsonmh reviewed Apr 30, 2019
View reviewed changes
johnsonmh
johnsonmh reviewed Apr 30, 2019
View reviewed changes
johnsonmh
johnsonmh reviewed Apr 30, 2019
View reviewed changes
johnsonmh
johnsonmh reviewed Apr 30, 2019
View reviewed changes
johnsonmh
johnsonmh reviewed Apr 30, 2019
View reviewed changes
johnsonmh
johnsonmh reviewed Apr 30, 2019
View reviewed changes
johnsonmh
johnsonmh reviewed Apr 30, 2019
View reviewed changes
@googlebot
Copy link

googlebot commented Apr 30, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

googlebot commented Apr 30, 2019

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

googlebot commented Jun 4, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jun 4, 2019
@googlebot
Copy link

googlebot commented Jun 4, 2019

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Jun 4, 2019
@googlebot
Copy link

googlebot commented Jun 4, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jun 4, 2019
@clocksmith
Copy link
Contributor Author

clocksmith commented Jun 4, 2019

Most comments resolved, PTAL

HansMuller
HansMuller approved these changes Jun 5, 2019
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

In places that test boolean values, either verify that we're ensuring that the value will be non-null or test == true or == false.

packages/flutter/lib/src/material/range_slider.dart Outdated
Copy link
Contributor

@HansMuller HansMuller Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my mistake. Using an AnimatedBuilder fails because the subtree doesn't change when the AnimatedBuilder's builder runs, since the subtree is just the same RenderWidget.

@clocksmith clocksmith merged commit e1d0ded into flutter:master Jun 6, 2019
tango5614 added a commit to tango5614/flutter that referenced this pull request Jun 7, 2019
@tango5614
Merge branch 'master' into fix28096
3da859d 
* master: (25 commits)
  Increase daemon protocol version for getSupportedPlatforms (flutter#33980)
  skip web test on crazy import (flutter#34017)
  Compatibility pass on flutter/foundation tests for JavaScript compilation. (1) (flutter#33349)
  0602dbb Roll src/third_party/dart 9dcb026b26..6e0d978505 (72 commits) (flutter#34027)
  Add chrome stable to dockerfile and web shard (flutter#33787)
  Codegen an entrypoint for flutter web applications (flutter#33956)
  Revert "Reland "Text inline widgets, TextSpan rework" (flutter#33946)" (flutter#34002)
  Revert "Re-add deprecated method for plugin migration compatibility. (flutter#34006)" (flutter#34022)
  Remove print (flutter#34004)
  Manual roll the engine to land the timing API (flutter#33989)
  Make plugins Swift-first on macOS (flutter#33997)
  Re-add deprecated method for plugin migration compatibility. (flutter#34006)
  make sure version check includes hotfixes (flutter#33459)
  Respond to AndroidView focus events. (flutter#33901)
  Add 'doctor' support for Windows (flutter#33872)
  Add use_frameworks to macOS Podfile template (flutter#33987)
  [Material] Create a themable Range Slider (continuous and discrete) (flutter#31681)
  Updating names to correct versioning convention (flutter#33865)
  Whitelist adb.exe heap corruption exit code. (flutter#33951)
  [flutter_tool] Fix 'q' for Fuchsia profile/debug mode (flutter#33846)
  ...
@clocksmith clocksmith mentioned this pull request Jun 10, 2019
Merged
8 tasks
clocksmith added a commit that referenced this pull request Jun 10, 2019
@clocksmith
[Material] Fix slider track shape to rounded (#34178)
0bbac72 
The slider track shape for the onPrimaryColors constructor should be rounded, but it was accidentally changed to rectangular in #31681. This change restores the original behavior. This only affects sliders that are themed with SliderThemeData.onPrimaryColors().
goderbauer
goderbauer reviewed Jun 13, 2019
View reviewed changes
packages/flutter/test/material/slider_theme_test.dart
expect(description, <String>[
'trackHeight: 7.0',
'activeTrackColor: Color(0xff000001)',
'activeTrackColor: Color(0xff000001)',
Copy link
Member

@goderbauer goderbauer Jun 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming it's not intentional that activeTrackColor is printed twice? I am removing it in #34012.

@clocksmith clocksmith mentioned this pull request Jun 19, 2019
Closed
@kirill-chirkov-at-clouty kirill-chirkov-at-clouty mentioned this pull request Jul 19, 2019
Closed
@clocksmith clocksmith deleted the range-slider-2 branch September 12, 2019 16:55
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@goderbauer goderbauer goderbauer left review comments

@johnsonmh johnsonmh johnsonmh left review comments

@willlarche willlarche Awaiting requested review from willlarche

+2 more reviewers

@HansMuller HansMuller HansMuller approved these changes

@rami-a rami-a rami-a approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@clocksmith clocksmith

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@clocksmith @googlebot @lpongetti @goderbauer @HansMuller @rami-a @johnsonmh