Skip to content

Conversation

@werainkhatri
Copy link
Member

@werainkhatri werainkhatri commented Feb 26, 2023

design doc: https://docs.flutter.dev/go/permissible-slider-interaction

closes #113370

open questions:

  • Should this be added to debugFillProperties? I see only a few things existing in that list.
  • Any semantic config change needed? Now that SliderInteraction.none is handled differently than onChanged being null.
    • No, as SliderInteraction.none doesn't exist anymore.
  • Should Slider.allowedInteraction be added to SliderTheme too?
    • Yes (done)
  • Should SliderInteraction.slideThumb use the actual thumb rect or overlay rect to determine if the user has tapped down on the “thumb”?
  • Should SliderInteraction.slideOnTrack be renamed to SliderInteraction.slideOnly coz its essentially that?
    • Yes.
  • Any other name suggestions for -?
    • The enum
      • SliderInteraction
      • SliderAction
    • Slider property
      • Slider.allowedInteraction
      • Slider.permissibleInteraction
      • Slider.interaction
      • Slider.allowedAction
      • Slider.permittedAction

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.

@werainkhatri werainkhatri requested a review from Piinks February 26, 2023 14:37
@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Feb 26, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

@werainkhatri werainkhatri Feb 28, 2023

Choose a reason for hiding this comment

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

ohh that's interesting, thanks for pointing that out.

EDIT: after looking into it, i realized that i'm not examining the enum at one place, rather evaluating different values in different places depending on the usecase. i can move the == checks to the if statements below if that'll make it more consistent with the style guide. lemme know your thoughts.

Copy link
Contributor

@Piinks Piinks Mar 17, 2023

Choose a reason for hiding this comment

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

The if/else statements should really be converted to switch statements. That way it is clear what is/isn't happening for each type of interaction, and more importantly, this will ensure the analyzer warns us if we ever change the values of the enum

Copy link
Member Author

Choose a reason for hiding this comment

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

done ✅, PTAL.

@Piinks
Copy link
Contributor

Piinks commented Feb 27, 2023

@TahaTesser may also have some thoughts on this

@werainkhatri werainkhatri requested a review from TahaTesser March 5, 2023 09:14
@TahaTesser
Copy link
Member

@werainkhatri
Are you planning to update this to include the switch statement?s

@werainkhatri
Copy link
Member Author

Are you planning to update this to include the switch statement?s

yes! have been busy with work. will do it soon.

@werainkhatri werainkhatri force-pushed the slider-interaction branch 2 times, most recently from 294e81a to 8a11c04 Compare April 8, 2023 11:31
@werainkhatri werainkhatri requested a review from Piinks April 8, 2023 13:10
@TahaTesser
Copy link
Member

@werainkhatri
Just tried the latest commit. Overall this looks good.
Could you please update your description to reflect the progress and remove anything not relevant anymore?

Should Slider.allowedInteraction be added to SliderTheme too?

I don't see a reason to not add this to the SliderTheme. Would be nice to theme slider interactions.

@werainkhatri
Copy link
Member Author

thank you so much! glad to hear that. will do the same by the weekend as I'm travelling now.

@werainkhatri werainkhatri marked this pull request as ready for review April 14, 2023 23:41
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.

This looks very close! Can you update the docs a bit more?

@werainkhatri
Copy link
Member Author

thanks for the review, and fixing the style.

could you help me with this from the description?

  • Should this be added to debugFillProperties? I see only a few things existing in that list.

also, are the names of classes and arguments okay?

@werainkhatri werainkhatri requested a review from Piinks April 18, 2023 21:25
@TahaTesser
Copy link
Member

thanks for the review, and fixing the style.

could you help me with this from the description?

  • Should this be added to debugFillProperties? I see only a few things existing in that list.

also, are the names of classes and arguments okay?

You can add them as it is, we can always edit the names before finalizing this.

Comment on lines 1909 to 1910
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(value, expectYesOrNo ? equals(0.5) : isNot(equals(0.5)),
reason: 'allowedInteraction is $whileTesting');
expect(value, expectYesOrNo ? equals(0.5) : isNot(equals(0.5)),
reason: 'allowedInteraction is $whileTesting');

This is a bit hard to read.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. The expectYesOrNo is difficult to follow. It is ok for tests to be a bit more verbose and written out for each test case.

Copy link
Member Author

@werainkhatri werainkhatri Apr 26, 2023

Choose a reason for hiding this comment

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

yeah that makes sense. sorry about that. i went overboard with the DRY principle, but at the cost of readability. improved it now.

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.

Looks like there is just some semantic refactoring in the tests left to do here. Thanks very much for your patience on this!

@Piinks Piinks force-pushed the slider-interaction branch from ff4bfe8 to 21ca4e8 Compare April 24, 2023 23:38
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 1, 2023
@werainkhatri werainkhatri deleted the slider-interaction branch May 1, 2023 18:21
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 1, 2023
tarrinneal pushed a commit to flutter/packages that referenced this pull request May 1, 2023
Manual roll Flutter from 66fa4c5 to 828a040 (79 revisions)

Manual roll requested by [email protected]

flutter/flutter@66fa4c5...828a040

2023-05-01 [email protected] Roll Flutter Engine from
666bc34c61aa to 687f4c761db1 (2 revisions) (flutter/flutter#125818)
2023-05-01 [email protected] Revert "Add
migrator to upgrade gradle version when conflict with And…
(flutter/flutter#125813)
2023-05-01 [email protected] Roll pub packages
(flutter/flutter#125801)
2023-05-01 [email protected] [tools] fix `expect` calls in
`FakeCommand` (flutter/flutter#125783)
2023-05-01 [email protected] Roll Packages from
7e3f5da to de6131d (41 revisions) (flutter/flutter#125811)
2023-05-01 [email protected] Introduce `TabBar.tabAlignment`
(flutter/flutter#125036)
2023-05-01 [email protected] Roll Flutter Engine from
b0da68e7e024 to 666bc34c61aa (1 revision) (flutter/flutter#125805)
2023-05-01 [email protected] add support to
customize Slider interacivity (flutter/flutter#121483)
2023-05-01 [email protected] Roll Flutter Engine from
b4551c72487c to b0da68e7e024 (1 revision) (flutter/flutter#125800)
2023-05-01 [email protected] Roll Flutter Engine from
605528f293d0 to b4551c72487c (1 revision) (flutter/flutter#125795)
2023-05-01 [email protected] Roll Flutter Engine from
bba66b658cee to 605528f293d0 (2 revisions) (flutter/flutter#125793)
2023-05-01 [email protected] Roll Flutter Engine from
2fa61b91d7c2 to bba66b658cee (1 revision) (flutter/flutter#125791)
2023-05-01 [email protected] Roll Flutter Engine from
30c91b8180e7 to 2fa61b91d7c2 (1 revision) (flutter/flutter#125789)
2023-05-01 [email protected] Roll Flutter Engine from
d76a22e67eea to 30c91b8180e7 (1 revision) (flutter/flutter#125787)
2023-05-01 [email protected] [tools] Apply Android Studio version
detection logic to explicitly configured installation directory
(`flutter config --android-studio-dir`) (flutter/flutter#125596)
2023-04-30 [email protected] Roll Flutter Engine from
f234d5e1dd26 to d76a22e67eea (1 revision) (flutter/flutter#125776)
2023-04-30 [email protected] Roll Flutter Engine from
c796390d14cb to f234d5e1dd26 (1 revision) (flutter/flutter#125773)
2023-04-30 [email protected] Roll Flutter Engine from
e99f31f4437d to c796390d14cb (1 revision) (flutter/flutter#125762)
2023-04-30 [email protected] Roll Flutter Engine from
1942b0c2cd9a to e99f31f4437d (1 revision) (flutter/flutter#125758)
2023-04-30 [email protected] Roll Flutter Engine from
7806f8a4fb4c to 1942b0c2cd9a (1 revision) (flutter/flutter#125757)
2023-04-29 [email protected] Roll Flutter Engine from
8167f909bc8d to 7806f8a4fb4c (2 revisions) (flutter/flutter#125750)
2023-04-29 [email protected] Roll Flutter Engine from
900b8a89b73b to 8167f909bc8d (1 revision) (flutter/flutter#125748)
2023-04-29 [email protected] Roll Flutter Engine from
c56ea398b0dc to 900b8a89b73b (1 revision) (flutter/flutter#125747)
2023-04-29 [email protected] Roll Flutter Engine from
0834c886f06a to c56ea398b0dc (1 revision) (flutter/flutter#125746)
2023-04-29 [email protected] Roll Flutter Engine from
68f2ed0a1db5 to 0834c886f06a (1 revision) (flutter/flutter#125736)
2023-04-29 [email protected] Roll Flutter Engine from
0079bb4a20d0 to 68f2ed0a1db5 (1 revision) (flutter/flutter#125735)
2023-04-29 [email protected] Fix crasher in DragableScrollableSheet
when controller is animating and switching widgets
(flutter/flutter#125721)
2023-04-29 [email protected] Roll Flutter Engine from
8f04b29c1b98 to 0079bb4a20d0 (2 revisions) (flutter/flutter#125734)
2023-04-29 [email protected] Roll Flutter Engine from
788d0ed5ed06 to 8f04b29c1b98 (1 revision) (flutter/flutter#125731)
2023-04-29 [email protected] Roll Flutter Engine from
89a8affdced0 to 788d0ed5ed06 (1 revision) (flutter/flutter#125729)
2023-04-29 [email protected] Roll Flutter Engine from
3835d975c8b0 to 89a8affdced0 (2 revisions) (flutter/flutter#125725)
2023-04-29 [email protected] Roll Flutter Engine from
1ae848ce6b55 to 3835d975c8b0 (1 revision) (flutter/flutter#125722)
2023-04-29 [email protected] fix package template
create platform folders (flutter/flutter#125292)
2023-04-28 [email protected] Sliver Cross Axis Group
(flutter/flutter#123862)
2023-04-28 [email protected] Roll Flutter Engine from
2a84ea55e4ef to 1ae848ce6b55 (1 revision) (flutter/flutter#125718)
2023-04-28 [email protected] Remove bringup from
new_gallery_skia_ios__transition_perf (flutter/flutter#125715)
2023-04-28 [email protected] Roll Flutter Engine from
98b6fabc66bb to 2a84ea55e4ef (10 revisions) (flutter/flutter#125714)
2023-04-28 [email protected] Opt into
CMake policy CMP0135 (flutter/flutter#125502)
2023-04-28 [email protected] Add a channel to query the engine
keyboard state (flutter/flutter#122885)
2023-04-28 [email protected] Roll pub packages
(flutter/flutter#125698)
2023-04-28 [email protected]
`Checkbox.fillColor` should be applied to checkbox's background color
when it is unchecked. (flutter/flutter#125643)
2023-04-28 [email protected] Add back one Skia test on
iOS (flutter/flutter#125663)
2023-04-28 [email protected] Roll pub packages
(flutter/flutter#125447)
2023-04-28 [email protected] Nit:
grammar in documentation (flutter/flutter#125462)
...
nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023
)

Manual roll Flutter from 66fa4c5 to 828a040 (79 revisions)

Manual roll requested by [email protected]

flutter/flutter@66fa4c5...828a040

2023-05-01 [email protected] Roll Flutter Engine from
666bc34c61aa to 687f4c761db1 (2 revisions) (flutter/flutter#125818)
2023-05-01 [email protected] Revert "Add
migrator to upgrade gradle version when conflict with And…
(flutter/flutter#125813)
2023-05-01 [email protected] Roll pub packages
(flutter/flutter#125801)
2023-05-01 [email protected] [tools] fix `expect` calls in
`FakeCommand` (flutter/flutter#125783)
2023-05-01 [email protected] Roll Packages from
7e3f5da to de6131d (41 revisions) (flutter/flutter#125811)
2023-05-01 [email protected] Introduce `TabBar.tabAlignment`
(flutter/flutter#125036)
2023-05-01 [email protected] Roll Flutter Engine from
b0da68e7e024 to 666bc34c61aa (1 revision) (flutter/flutter#125805)
2023-05-01 [email protected] add support to
customize Slider interacivity (flutter/flutter#121483)
2023-05-01 [email protected] Roll Flutter Engine from
b4551c72487c to b0da68e7e024 (1 revision) (flutter/flutter#125800)
2023-05-01 [email protected] Roll Flutter Engine from
605528f293d0 to b4551c72487c (1 revision) (flutter/flutter#125795)
2023-05-01 [email protected] Roll Flutter Engine from
bba66b658cee to 605528f293d0 (2 revisions) (flutter/flutter#125793)
2023-05-01 [email protected] Roll Flutter Engine from
2fa61b91d7c2 to bba66b658cee (1 revision) (flutter/flutter#125791)
2023-05-01 [email protected] Roll Flutter Engine from
30c91b8180e7 to 2fa61b91d7c2 (1 revision) (flutter/flutter#125789)
2023-05-01 [email protected] Roll Flutter Engine from
d76a22e67eea to 30c91b8180e7 (1 revision) (flutter/flutter#125787)
2023-05-01 [email protected] [tools] Apply Android Studio version
detection logic to explicitly configured installation directory
(`flutter config --android-studio-dir`) (flutter/flutter#125596)
2023-04-30 [email protected] Roll Flutter Engine from
f234d5e1dd26 to d76a22e67eea (1 revision) (flutter/flutter#125776)
2023-04-30 [email protected] Roll Flutter Engine from
c796390d14cb to f234d5e1dd26 (1 revision) (flutter/flutter#125773)
2023-04-30 [email protected] Roll Flutter Engine from
e99f31f4437d to c796390d14cb (1 revision) (flutter/flutter#125762)
2023-04-30 [email protected] Roll Flutter Engine from
1942b0c2cd9a to e99f31f4437d (1 revision) (flutter/flutter#125758)
2023-04-30 [email protected] Roll Flutter Engine from
7806f8a4fb4c to 1942b0c2cd9a (1 revision) (flutter/flutter#125757)
2023-04-29 [email protected] Roll Flutter Engine from
8167f909bc8d to 7806f8a4fb4c (2 revisions) (flutter/flutter#125750)
2023-04-29 [email protected] Roll Flutter Engine from
900b8a89b73b to 8167f909bc8d (1 revision) (flutter/flutter#125748)
2023-04-29 [email protected] Roll Flutter Engine from
c56ea398b0dc to 900b8a89b73b (1 revision) (flutter/flutter#125747)
2023-04-29 [email protected] Roll Flutter Engine from
0834c886f06a to c56ea398b0dc (1 revision) (flutter/flutter#125746)
2023-04-29 [email protected] Roll Flutter Engine from
68f2ed0a1db5 to 0834c886f06a (1 revision) (flutter/flutter#125736)
2023-04-29 [email protected] Roll Flutter Engine from
0079bb4a20d0 to 68f2ed0a1db5 (1 revision) (flutter/flutter#125735)
2023-04-29 [email protected] Fix crasher in DragableScrollableSheet
when controller is animating and switching widgets
(flutter/flutter#125721)
2023-04-29 [email protected] Roll Flutter Engine from
8f04b29c1b98 to 0079bb4a20d0 (2 revisions) (flutter/flutter#125734)
2023-04-29 [email protected] Roll Flutter Engine from
788d0ed5ed06 to 8f04b29c1b98 (1 revision) (flutter/flutter#125731)
2023-04-29 [email protected] Roll Flutter Engine from
89a8affdced0 to 788d0ed5ed06 (1 revision) (flutter/flutter#125729)
2023-04-29 [email protected] Roll Flutter Engine from
3835d975c8b0 to 89a8affdced0 (2 revisions) (flutter/flutter#125725)
2023-04-29 [email protected] Roll Flutter Engine from
1ae848ce6b55 to 3835d975c8b0 (1 revision) (flutter/flutter#125722)
2023-04-29 [email protected] fix package template
create platform folders (flutter/flutter#125292)
2023-04-28 [email protected] Sliver Cross Axis Group
(flutter/flutter#123862)
2023-04-28 [email protected] Roll Flutter Engine from
2a84ea55e4ef to 1ae848ce6b55 (1 revision) (flutter/flutter#125718)
2023-04-28 [email protected] Remove bringup from
new_gallery_skia_ios__transition_perf (flutter/flutter#125715)
2023-04-28 [email protected] Roll Flutter Engine from
98b6fabc66bb to 2a84ea55e4ef (10 revisions) (flutter/flutter#125714)
2023-04-28 [email protected] Opt into
CMake policy CMP0135 (flutter/flutter#125502)
2023-04-28 [email protected] Add a channel to query the engine
keyboard state (flutter/flutter#122885)
2023-04-28 [email protected] Roll pub packages
(flutter/flutter#125698)
2023-04-28 [email protected]
`Checkbox.fillColor` should be applied to checkbox's background color
when it is unchecked. (flutter/flutter#125643)
2023-04-28 [email protected] Add back one Skia test on
iOS (flutter/flutter#125663)
2023-04-28 [email protected] Roll pub packages
(flutter/flutter#125447)
2023-04-28 [email protected] Nit:
grammar in documentation (flutter/flutter#125462)
...
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
auto-submit bot pushed a commit that referenced this pull request Oct 18, 2023
… on `SliderInteraction.slideOnly` allowed interaction (#136720)

fixes [Slider will call onChanged before onChangeStart when sliding.](#136707)

This fixes a regression from #121483

### Code sample

<details>
<summary>expand to view the code sample</summary> 

```dart
import 'package:flutter/material.dart';

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

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

  @OverRide
  Widget build(BuildContext context) {
    return const MaterialApp(
      debugShowCheckedModeBanner: false,
      home: Example(),
    );
  }
}

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

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

class _ExampleState extends State<Example> {
  double _sliderValue = 0.5;

  @OverRide
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Slider(
          // allowedInteraction: SliderInteraction.tapAndSlide,
          // allowedInteraction: SliderInteraction.tapOnly,
          // allowedInteraction: SliderInteraction.slideOnly
          // allowedInteraction: SliderInteraction.slideThumb,
          value: _sliderValue,
          onChangeStart: (newValue) {
            print("onChangeStart ......");
          },
          onChanged: (newValue) {
            print("onChanged ......");
            setState(() {
              _sliderValue = newValue;
            });
          },
          onChangeEnd: (newValue) {
            print("onChangeEnd ......");
          },
        ),
      ),
    );
  }
}
```

</details>
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 c: new feature Nothing broken; request for a new capability f: gestures flutter/packages/flutter/gestures repository. f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow Slider to be "slide only" like YouTube mobile progress bar

3 participants