Skip to content
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

Add Material 3 support for Slider - Part 2 #114624

Merged
merged 4 commits into from Nov 17, 2022

Conversation

TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented Nov 3, 2022

Closes #111451

Description

This PR adds Material 3 value indicator shape to Slider.

  1. There is no official name that I could find for the M3 value indicator so I decided to call it DropSliderValueIndicatorShape as it resembles a water drop. (M2 value indicator is RectangularSliderValueIndicatorShape and it looks like a rectangular box).

  2. DropSliderValueIndicatorShape is based on existing code from RectangularSliderValueIndicatorShape.

  3. Added tests to check the default value indicator.

code sample
import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      themeMode: ThemeMode.light,
      theme: ThemeData(
        useMaterial3: true,
        brightness: Brightness.light,
        colorSchemeSeed: const Color(0xff6750a4),
        sliderTheme: const SliderThemeData(
          showValueIndicator: ShowValueIndicator.always,
        ),
      ),
      darkTheme: ThemeData(
        useMaterial3: true,
        brightness: Brightness.dark,
        colorSchemeSeed: const Color(0xff6750a4),
        sliderTheme: const SliderThemeData(
          showValueIndicator: ShowValueIndicator.always,
        ),
      ),
      home: const SliderExample(),
    );
  }
}

class SliderExample extends StatefulWidget {
  const SliderExample({super.key});

  @override
  State<SliderExample> createState() => _SliderExampleState();
}

class _SliderExampleState extends State<SliderExample> {
  double sliderValue1 = 0.0;
  double sliderValue2 = 100.0;
  double sliderValue3 = 0.0;
  double sliderValue4 = 10.0;

  @override
  Widget build(BuildContext context) {

    return Scaffold(
      appBar: AppBar(
        title: const Text('Slider Example'),
      ),
      body: Column(
        mainAxisAlignment: MainAxisAlignment.center,
        children: <Widget>[
          Slider(
            value: sliderValue1,
            max: 11,
            label: sliderValue1.round().toString(),
            onChanged: (double value) {
              setState(() {
                sliderValue1 = value;
              });
            },
          ),
          const SizedBox(height: 50),
          Slider(
            value: sliderValue2,
            min: 100,
            max: 500,
            divisions: 50,
            label: sliderValue2.round().toString(),
            onChanged: (double value) {
              setState(() {
                sliderValue2 = value;
              });
            },
          ),
          const SizedBox(height: 50),
          Slider(
            value: sliderValue3,
            max: 5.74,
            divisions: 23,
            label: sliderValue3.toStringAsFixed(2),
            onChanged: (double value) {
              setState(() {
                sliderValue3 = value;
              });
            },
          ),
          const SizedBox(height: 50),
          Slider(
            value: sliderValue4,
            min: 10,
            max: 50,
            divisions: 20,
            label: '${sliderValue4.toStringAsFixed(1)}K',
            onChanged: (double value) {
              setState(() {
                sliderValue4 = value;
              });
            },
          ),
        ],
      ),
    );
  }
}

Preview

Flutter

Single digit Triple digits With decimals Longer value

Android

Single digit Triple digits With decimals Longer value

Filing as a draft to confirm the implementation and the name.
Once confirmed I will add golden tests under test/material/value_indicating_slider_test.dart.
Done.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels Nov 3, 2022
@Piinks
Copy link
Contributor

Piinks commented Nov 7, 2022

image

@TahaTesser it looks like this triple digit one has clipping along the bottom of the digits, can you take a look?

@TahaTesser
Copy link
Member Author

image

@TahaTesser it looks like this triple digit one has clipping along the bottom of the digits, can you take a look?

Great catch! Just fixed

Screenshot 2022-11-08 at 11 58 46

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

I think this looks great! I am checking in with @darrenaustin about the question on the drop name. 🙂 💧

@TahaTesser
Copy link
Member Author

TahaTesser commented Nov 10, 2022

@Piinks it was just first name that popped in my head as I was expecting something more official from team.

Another name idea is 'PinValueIndicatorShape' as it looks like map pin (naming is difficult 😀)

@Piinks
Copy link
Contributor

Piinks commented Nov 10, 2022

(naming is difficult 😀)

I hear that! Always a struggle for me. :)

@TahaTesser
Copy link
Member Author

In my Android testing, I notice this particular shape in Slider is called TooltipDrawable but it sounds very generic and might be confused with tooltip widget shape.

@TahaTesser TahaTesser marked this pull request as ready for review November 16, 2022 11:59
@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #114624 at sha 964bb91

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Nov 16, 2022
@TahaTesser
Copy link
Member Author

Added golden tests, this is ready for review.

@Piinks
Copy link
Contributor

Piinks commented Nov 16, 2022

There aren't any images showing up, I wonder if it is because all of the commits here are forced pushed. Can you create a second commit and push it? I see there are 60 images on the backend, but they are all attributed to the same patchset, which may be causing the issue. I will be able to follow up with the Gold team tomorrow.

https://flutter-gold.skia.org/json/v1/changelist_summary/github/114624

Copy link
Contributor

@darrenaustin darrenaustin left a comment

Choose a reason for hiding this comment

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

flutter-lgtm

/// * [Slider], which includes a value indicator defined by this shape.
/// * [SliderTheme], which can be used to configure the slider value indicator
/// of all sliders in a widget subtree.
class DropSliderValueIndicatorShape extends SliderComponentShape {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to say that the theme file is an odd place for this, but given that all the other shapes are defined in this file it would be good to keep them together. I think the name is fine. I checked with the Material team and they didn't seem to have a canonical name that we could find, so DropSliderValueIndicatorShape it is 😎.

Copy link
Member Author

Choose a reason for hiding this comment

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

Appreciate it!

);

} finally {
debugDisableShadows = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

@TahaTesser
Copy link
Member Author

Can you create a second commit and push it?

Sure! just pushed an empty commit.

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #114624 at sha 93f327a

@Piinks
Copy link
Contributor

Piinks commented Nov 16, 2022

That worked! Thanks! Can you take a peek and see if they look as expected?

https://flutter-gold.skia.org/search?issue=114624&crs=github&patchsets=2&corpus=flutter

Any text is swapped out for little boxes, that is expected

@TahaTesser
Copy link
Member Author

There seems to be a drawing bug in the HTML renderer where the drawing order doesn't render Path after RRect but Canvaskit renders fine regardless of the order.

Notice this in the golden images.

Updating the drawing order fixes the issue.

Before Fix

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #114624 at sha bf6163e

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

LGTM-stamp

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 17, 2022
@auto-submit auto-submit bot merged commit ac06523 into flutter:master Nov 17, 2022
122 checks passed
@TahaTesser TahaTesser deleted the m3_slider_indicator branch November 17, 2022 19:18
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 17, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 18, 2022
* 70b7445cb Reland Added Badge.count constructor (flutter/flutter#115566)

* 31f863162 fa7e1965e [Impeller] Fix glyph atlas uploads and renders (flutter/engine#37691) (flutter/flutter#115556)

* a1ea383fa Label should always be aligned with text in filled input decoration (flutter/flutter#115540)

* c2b29501f Add selection feedback for both selection area and text field (flutter/flutter#115373)

* 034440761 Rev package:pub_semver to the latest version (flutter/flutter#115570)

* ac06523b7 Add Material 3 support for `Slider` - Part 2  (flutter/flutter#114624)

* b181d07e0 a2fa4e9 cirrus to luci (flutter/plugins#6711) (flutter/flutter#115573)

* e1efd0d39 b241e69fd [ui] reland add docs to FragmentShader (flutter/engine#37699) (flutter/flutter#115578)

* efb069474 Remove unused flutter_attach_test_fuchsia (flutter/flutter#115515)

* a5a368cbf 487ee66f6 [macOS] Merge FlutterRenderer and implementation (flutter/engine#37696) (flutter/flutter#115581)

* 4ff7fc641 Fixes a bug where dragging a collapsed handle in TextField does not vibrate (flutter/flutter#115586)

* 20be280cc da9534ea6 [macOS] Consolidate external texture classes (flutter/engine#37703) (flutter/flutter#115585)

* 8a7102e7e Roll Flutter Engine from da9534ea6534 to d955a72c5604 (3 revisions) (flutter/flutter#115589)

* e1903a2ad Roll Flutter Engine from d955a72c5604 to 1e1a4ab3c993 (4 revisions) (flutter/flutter#115592)

* 78390a086 Roll Flutter Engine from 1e1a4ab3c993 to b65c24ce621a (2 revisions) (flutter/flutter#115598)

* 75a0a7255 [devicelab] measure entire release folder size, zipped (flutter/flutter#115597)

* 59a01b64f Roll Flutter Engine from b65c24ce621a to 49b52db603cc (3 revisions) (flutter/flutter#115606)

* ec03f1c8c Revert "[devicelab] measure entire release folder size, zipped (#115597)" (flutter/flutter#115609)

* 710e708cc Improve showSnackBar documentation (flutter/flutter#114612)

* 915c3deb6 Roll Flutter Engine from 49b52db603cc to 80b25a302b4c (2 revisions) (flutter/flutter#115608)

* 450f16245 Roll Flutter Engine from 80b25a302b4c to e812122e4060 (2 revisions) (flutter/flutter#115614)

* 0b33b8592 [devicelab] measure entire release folder size, zipped (flutter/flutter#115612)

* 9379c3233 Revert "[devicelab] measure entire release folder size, zipped (#115612)" (flutter/flutter#115617)

* b746557a3 f27666d2f [macOS] Merge FlutterBackingStore implementations (flutter/engine#37730) (flutter/flutter#115616)

* 5487a7deb Roll Flutter Engine from f27666d2f4da to 39f546585b0b (2 revisions) (flutter/flutter#115618)

* f261c2f71 update comments (flutter/flutter#115603)

* 9c9f7818a 04aea3c47 iOS PlatformView only sets a maskView when necessary (flutter/engine#37434) (flutter/flutter#115621)

* 69269602e 4ca2c1d78 Roll Skia from 55f654bf5cff to 9d56e506b4df (13 revisions) (flutter/engine#37739) (flutter/flutter#115625)

* de4c0b19a Use `double.isNaN` instead of `... == double.nan` (which is always false) (flutter/flutter#115424)

* a655f8542 a62736769 Roll Skia from 9d56e506b4df to d693b4b9fe5e (5 revisions) (flutter/engine#37741) (flutter/flutter#115640)

* 18c87274c f092cd826 Roll Fuchsia Mac SDK from SVtX810D2U_ZgBcpx... to tklUfTsSOVKk49tYq... (flutter/engine#37742) (flutter/flutter#115643)
shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
* Add Material 3 support for Slider - Part 2

* Kick tests

* Update drawing order to fix html renderer bug

* Update test
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
* Add Material 3 support for Slider - Part 2

* Kick tests

* Update drawing order to fix html renderer bug

* Update test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. will affect goldens Changes to golden files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Slider to support Material 3
3 participants