Skip to content

Conversation

@838
Copy link
Contributor

@838 838 commented Oct 4, 2023

Consider a scenario where the background color and indicator's background color are the same.

Adding a stroke color to the value indicator would be a valuable for the following reasons:

  • Visual Clarity: It would allow developers to make the value indicator stand out more against the background, making it easier for users to notice.
  • Customization: It would provide more flexibility in customizing the appearance of the sliding widget, allowing developers to match the design requirements of their apps.
  • Accessibility: Improved visual distinction can enhance the accessibility of the sliding widget for users with various needs.

List which issues are fixed by this PR. You must list at least one issue.

Fixes #135984

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.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Oct 4, 2023
@TahaTesser TahaTesser self-requested a review October 5, 2023 07:03
@TahaTesser
Copy link
Member

TahaTesser commented Oct 5, 2023

@838
Thanks for contributing!

Overall this looks great.

We should've one test that checks the Slider using RectangularSliderValueIndicatorShape with the new valueIndicatorStrokeColor property is actually rendering the value indicator shape with stroke as expected.

@838
Copy link
Contributor Author

838 commented Oct 6, 2023

@TahaTesser
Thanks for the feedback 😊.

The test is now added.

@838 838 requested a review from TahaTesser October 6, 2023 11:59
Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

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

LGTM! We'll need a second review from @Piinks.

@TahaTesser TahaTesser requested a review from Piinks October 6, 2023 12:59
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.

Hi @838 welcome! Thank you for contributing.

@TahaTesser
Copy link
Member

@838
Looks like the changes are failing some tests in the slider_theme_test.dart class

@838 838 requested review from Piinks and TahaTesser October 16, 2023 10:05
@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 #135986 at sha 7a4bf56

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Oct 16, 2023
Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

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

Good job with the updates.

While reviewing your PR, I've created code sample to check all the shapes you've updated here

Code sample

expand to view the 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,
      theme: ThemeData.dark(useMaterial3: true),
      home: const Example(),
    );
  }
}

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

  @override
  State<Example> createState() => _ExampleState();
}

class _ExampleState extends State<Example> {
  double _sliderValue = 0.5;
  RangeValues _rangeSliderValues = const RangeValues(0.25, 0.75);

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Sliders'),
      ),
      body: Column(
        mainAxisAlignment: MainAxisAlignment.spaceEvenly,
        children: [
          _buildSlider(range: false),
          _buildSlider(
            range: false,
            rectangularSliderValueIndicatorShape:
                const RectangularSliderValueIndicatorShape(),
          ),
          _buildSlider(
            range: false,
            paddleSliderValueIndicatorShape:
                const PaddleSliderValueIndicatorShape(),
          ),
          _buildSlider(
            range: true,
            rectangularRangeSliderValueIndicatorShape:
                const RectangularRangeSliderValueIndicatorShape(),
          ),
          _buildSlider(
            range: true,
            paddleRangeSliderValueIndicatorShape:
                const PaddleRangeSliderValueIndicatorShape(),
          ),
        ],
      ),
    );
  }

  Widget _buildSlider({
    bool range = false,
    RectangularSliderValueIndicatorShape? rectangularSliderValueIndicatorShape,
    PaddleSliderValueIndicatorShape? paddleSliderValueIndicatorShape,
    RectangularRangeSliderValueIndicatorShape? rectangularRangeSliderValueIndicatorShape,
    PaddleRangeSliderValueIndicatorShape? paddleRangeSliderValueIndicatorShape,
  }) {
    if (range) {
      return SliderTheme(
        data: SliderThemeData(
          rangeValueIndicatorShape: rectangularRangeSliderValueIndicatorShape ??
              paddleRangeSliderValueIndicatorShape,
          valueIndicatorColor: Theme.of(context).colorScheme.surface,
          valueIndicatorStrokeColor: Theme.of(context).colorScheme.tertiary,
          showValueIndicator: ShowValueIndicator.always,
          valueIndicatorTextStyle: TextStyle(
            color: Theme.of(context).colorScheme.onSurface,
          ),
        ),
        child: RangeSlider(
          values: _rangeSliderValues,
          onChanged: (value) {
            setState(() {
              _rangeSliderValues = value;
            });
          },
          labels: RangeLabels(
            _rangeSliderValues.start.round().toString(),
            _rangeSliderValues.end.round().toString(),
          ),
          min: 0,
          max: 1,
        ),
      );
    }
    return SliderTheme(
      data: SliderThemeData(
        valueIndicatorShape: rectangularSliderValueIndicatorShape ??
            paddleSliderValueIndicatorShape,
        valueIndicatorColor: Theme.of(context).colorScheme.surface,
        valueIndicatorStrokeColor: Theme.of(context).colorScheme.tertiary,
        showValueIndicator: ShowValueIndicator.always,
        valueIndicatorTextStyle: TextStyle(
          color: Theme.of(context).colorScheme.onSurface,
        ),
      ),
      child: Slider(
        value: _sliderValue,
        onChanged: (value) {
          setState(() {
            _sliderValue = value;
          });
        },
        label: _sliderValue.round().toString(),
        min: 0,
        max: 1,
      ),
    );
  }
}

sizeWithOverflow: sizeWithOverflow!,
backgroundPaintColor: sliderTheme!.valueIndicatorColor!,
strokePaintColor: isOnTop! ? sliderTheme.overlappingShapeStrokeColor : null,
strokePaintColor: isOnTop! ? sliderTheme.overlappingShapeStrokeColor : sliderTheme.valueIndicatorStrokeColor,
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you added tests for all the value indicator shapes except this RectangularRangeSliderValueIndicatorShape

..path(color: theme.sliderTheme.valueIndicatorColor),
);
});

Copy link
Member

Choose a reason for hiding this comment

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

RectangularRangeSliderValueIndicatorShape test here

@838 838 requested a review from TahaTesser October 17, 2023 15:45
@838
Copy link
Contributor Author

838 commented Oct 17, 2023

@TahaTesser

I appreciate your input ☺️.

Should I also add tests to verify that 'overlappingShapeStrokeColor' behaves correctly when a valueIndicatorStrokeColor is provided for RangeSlider?

@TahaTesser
Copy link
Member

Yes, in the same test class

@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 #135986 at sha 70cd074

@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 #135986 at sha 5bd1eef

await gesture.up();
});

testWidgetsWithLeakTracking('RectangularRangeSliderValueIndicatorShape supports SliderTheme.valueIndicatorStrokeColor on overlapping indicator', (WidgetTester tester) async {
Copy link
Member

Choose a reason for hiding this comment

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

Is this a duplicate test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not entirely duplicated.

In the scenario where RectangularRangeSliderValueIndicatorShape supports SliderTheme.valueIndicatorStrokeColor, the indicator values are set as 0 and 0.5 to ensure they do not overlap, and the overlappingShapeStrokeColor is left unset in SliderTheme.

However, in this particular test case, both indicator values are configured as 0, and the overlappingShapeStrokeColor is explicitly defined in SliderTheme to ensure overlappingShapeStrokeColor is used instead of the valueIndicatorStrokeColor

Copy link
Member

Choose a reason for hiding this comment

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

Good idea

await gesture.up();
});

testWidgetsWithLeakTracking('PaddleRangeSliderValueIndicatorShape supports SliderTheme.valueIndicatorStrokeColor on overlapping indicator', (WidgetTester tester) async {
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a duplicate too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as this comment but instead testing PaddleRangeSliderValueIndicatorShape

Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

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

LGTM!

We'll need a second review before we land this.

@Piinks
Copy link
Contributor

Piinks commented Oct 18, 2023

Goldens are approved!

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.

Flutter_LGTM
Thank you for the contribution!

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 18, 2023
@auto-submit auto-submit bot merged commit a956731 into flutter:master Oct 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 18, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 19, 2023
Manual roll Flutter from 6eea6e2 to 189196d (44 revisions)

Manual roll requested by [email protected]

flutter/flutter@6eea6e2...189196d

2023-10-18 [email protected] Roll Flutter Engine from ab86c53c19cd to 6caee3236d37 (2 revisions) (flutter/flutter#136834)
2023-10-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Reland] Skip injecting Bonjour settings when port publication is disabled" (flutter/flutter#136839)
2023-10-18 [email protected] Convert menus to use OverlayPortal (flutter/flutter#130534)
2023-10-18 [email protected] [Reland] Skip injecting Bonjour settings when port publication is disabled (flutter/flutter#136751)
2023-10-18 [email protected] Roll Flutter Engine from 0eff5d191856 to ab86c53c19cd (1 revision) (flutter/flutter#136832)
2023-10-18 [email protected] Fixes ability to call nextFocus() on a node to focus its descendant (flutter/flutter#136773)
2023-10-18 [email protected] Roll Flutter Engine from b778a07f8ae9 to 0eff5d191856 (6 revisions) (flutter/flutter#136829)
2023-10-18 [email protected] Implement GApplication:shutdown so a Flutter developer knows where to put code that should occur on application shutdown. (flutter/flutter#136780)
2023-10-18 [email protected] Roll Flutter Engine from 7f37c9b181af to b778a07f8ae9 (1 revision) (flutter/flutter#136818)
2023-10-18 [email protected] Fix `Slider` `onChanged` callback order & never calls `onChangeStart` on  `SliderInteraction.slideOnly` allowed interaction (flutter/flutter#136720)
2023-10-18 [email protected] Roll Flutter Engine from 78026b4003fe to 7f37c9b181af (1 revision) (flutter/flutter#136817)
2023-10-18 [email protected] [Feat] Stroke color for Slider value indicator (flutter/flutter#135986)
2023-10-18 [email protected] Fixed : Empty Rows shown at last page in Paginated data table (flutter/flutter#132646)
2023-10-18 [email protected] Roll Flutter Engine from 46923fd39032 to 78026b4003fe (1 revision) (flutter/flutter#136814)
2023-10-18 [email protected] Roll Packages from d439062 to 14aa69e (1 revision) (flutter/flutter#136813)
2023-10-18 [email protected] Don't build native assets in `flutter build bundle` (flutter/flutter#136641)
2023-10-18 [email protected] Roll Flutter Engine from c9c9684e03a3 to 46923fd39032 (1 revision) (flutter/flutter#136795)
2023-10-18 [email protected] Roll Flutter Engine from 0c1c29271e8b to c9c9684e03a3 (1 revision) (flutter/flutter#136792)
2023-10-18 [email protected] Roll Flutter Engine from 1de09d13e708 to 0c1c29271e8b (3 revisions) (flutter/flutter#136789)
2023-10-18 [email protected] Roll Flutter Engine from 6fc36e61a99a to 1de09d13e708 (1 revision) (flutter/flutter#136785)
2023-10-18 [email protected] Roll Flutter Engine from 3f818efff3c5 to 6fc36e61a99a (1 revision) (flutter/flutter#136781)
2023-10-18 [email protected] Roll Flutter Engine from 2eef9b43cfb4 to 3f818efff3c5 (3 revisions) (flutter/flutter#136777)
2023-10-18 [email protected] Roll Flutter Engine from 5df7af34a718 to 2eef9b43cfb4 (4 revisions) (flutter/flutter#136774)
2023-10-18 [email protected] Flutter preview device (flutter/flutter#135639)
2023-10-18 [email protected] Add findChildIndexCallback examples (flutter/flutter#133469)
2023-10-18 [email protected] Adds API for performing semantics actions in tests (flutter/flutter#132598)
2023-10-18 [email protected] Roll Flutter Engine from e57b5bac4244 to 5df7af34a718 (2 revisions) (flutter/flutter#136772)
2023-10-17 [email protected] Roll Flutter Engine from f9f937e51080 to e57b5bac4244 (3 revisions) (flutter/flutter#136768)
2023-10-17 [email protected] clean up `--dart-define-from-file` option tests (flutter/flutter#135980)
2023-10-17 [email protected] GoldenFileComparators should dispose created Image objects.  (flutter/flutter#136716)
2023-10-17 [email protected] make integration_test_driver_extended.dart support writeResponseData--(done) (flutter/flutter#128382)
2023-10-17 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 3.6.0 to 4.1.1 (flutter/flutter#136762)
2023-10-17 [email protected] Roll Flutter Engine from 289f29b1ad00 to f9f937e51080 (1 revision) (flutter/flutter#136755)
2023-10-17 [email protected] Roll Flutter Engine from 659e68a097b5 to 289f29b1ad00 (1 revision) (flutter/flutter#136752)
2023-10-17 [email protected] Support --web-header option for flutter run (flutter/flutter#136297)
2023-10-17 [email protected] Add Android 14 physical devices to firebase tests (flutter/flutter#136736)
2023-10-17 [email protected] Roll Flutter Engine from 62a90a91cee3 to 659e68a097b5 (3 revisions) (flutter/flutter#136746)
2023-10-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Skip injecting Bonjour settings when port publication is disabled" (flutter/flutter#136750)
2023-10-17 [email protected] Revert "[SingleChildScrollView] Correct the offset pixels if it is out of range during layout" (flutter/flutter#136744)
2023-10-17 [email protected] Skip injecting Bonjour settings when port publication is disabled (flutter/flutter#136562)
2023-10-17 [email protected] Roll Flutter Engine from 3ecbe924a598 to 62a90a91cee3 (2 revisions) (flutter/flutter#136739)
2023-10-17 [email protected] Roll Flutter Engine from 0a4d8b99a95b to 3ecbe924a598 (3 revisions) (flutter/flutter#136732)
2023-10-17 [email protected] Reenable NexusLowRes API 29 (flutter/flutter#136686)
2023-10-17 [email protected] Reenable the nexus 6p tests (flutter/flutter#136689)
...
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. will affect goldens Changes to golden files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[A11Y] More contrast in Slider widget value indicator

3 participants