Skip to content

Material Bottom Sheet Reveal/Dismiss animation uses a curved animation #51122

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
fluttergithubbot merged 21 commits into from
Mar 2, 2020
Merged

Material Bottom Sheet Reveal/Dismiss animation uses a curved animation #51122

fluttergithubbot merged 21 commits into from
Mar 2, 2020

Conversation

@guidezpl
Copy link
Member

@guidezpl guidezpl commented Feb 20, 2020 •
edited
Loading

Description

Ensures all bottom sheets use non-linear animations for entrance and exits, while still allowing dragging (linearly) bottom sheets. Fixes the animation durations to match the spec.

Before (in slow motion):
before

After (in slow motion):1
after

Related Issues

Closes #19469
Future work #51627

Tests

I added the following tests:

  • A test to ensure that persistent bottom sheets animate non-linearly
  • A test to ensure that modal bottom sheets animate non-linearly

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 read the Tree Hygiene wiki page, which explains my responsibilities.
  • 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

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Feb 20, 2020
perclasson
perclasson reviewed Feb 20, 2020
View reviewed changes
Copy link
Contributor

@perclasson perclasson left a comment

Choose a reason for hiding this comment

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

Some style nits and comments

Zazo032
Zazo032 reviewed Feb 20, 2020
View reviewed changes
rami-a
rami-a approved these changes Feb 20, 2020
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.

LGTM with a couple comments

rami-a
rami-a reviewed Feb 20, 2020
View reviewed changes
@Zazo032
Copy link
Contributor

Zazo032 commented Feb 21, 2020

@guidezpl the analyzer is unhappy about this:

▌15:22:08▐ Trailing spaces...
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
/tmp/flutter sdk/packages/flutter/lib/src/material/bottom_sheet.dart:500: trailing U+0020 space character
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

@guidezpl guidezpl requested a review from HansMuller February 21, 2020 10:53
HansMuller
HansMuller reviewed Feb 24, 2020
View reviewed changes
packages/flutter/lib/src/material/bottom_sheet.dart
final bool enableDrag;

/// Called when the user begins dragging the bottom sheet vertically.
final BottomSheetDragStartHandler onDragStart;
Copy link
Contributor

@HansMuller HansMuller Feb 24, 2020

Choose a reason for hiding this comment

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

The new callbacks should explain how they'd typically be used and how they relate to the existing onClosing and enableDrag parameters.

Copy link
Member Author

@guidezpl guidezpl Feb 25, 2020

Choose a reason for hiding this comment

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

Done

packages/flutter/lib/src/material/bottom_sheet.dart
/// straight line, and the top-right quarter will contain the entire contents of
/// [Curves.easeOut].
///
/// This is useful in situations where a widget must track the user's finger
Copy link
Contributor

@HansMuller HansMuller Feb 24, 2020

Choose a reason for hiding this comment

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

This curve appears to fit a common idiom - linear drag then cuved animated fling - that's not specific to BottomSheet. Rather than defining it as specific to BottomSheet now, and then later as a generic utility with a different name, can we keep the API private for now and then make it public later? We'd have to include a copy of _BottomSheetSuspendedCurve in scaffold.dart for now. Making this public would require adding some usage examples to the docs and maybe retrofitting some existing code to use it (and to prove that it's generally useful).

Copy link
Member Author

@guidezpl guidezpl Feb 25, 2020

Choose a reason for hiding this comment

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

I don't mind adding examples and putting it in material/curves.dart. I think this would be cleaner and a future PR could use it in more places that exhibit this kind of behaviour. What do you think?

Copy link
Contributor

@HansMuller HansMuller Feb 26, 2020

Choose a reason for hiding this comment

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

I think that expanding on this part of the PR, by adding a track-fling curve and some examples to curves.dart in a future PR, is a good idea. So long as the curve is (the two copies of the curve are) private now.

Copy link
Member Author

@guidezpl guidezpl Feb 28, 2020

Choose a reason for hiding this comment

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

Sounds good

@guidezpl guidezpl mentioned this pull request Feb 28, 2020
Closed
HansMuller
HansMuller approved these changes Mar 2, 2020
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

@fluttergithubbot fluttergithubbot merged commit ec64f93 into flutter:master Mar 2, 2020
@guidezpl guidezpl deleted the bottom-sheet-curve branch March 3, 2020 09:29
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@perclasson perclasson perclasson left review comments

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

@Zazo032 Zazo032 Zazo032 left review comments

@HansMuller HansMuller HansMuller approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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.

Material Bottom Sheet Reveal/Dismiss animation should use a curved animation

7 participants

@guidezpl @Zazo032 @HansMuller @perclasson @rami-a @googlebot @fluttergithubbot