CupertinoActionSheet #19232

nataliesampsell · 2018-07-10T23:09:38Z

-Add showCupertinoModalPopup, CupertinoActionSheet

…to action_sheets

gspencergoog · 2018-07-10T23:41:24Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+import 'colors.dart';
+import 'scrollbar.dart';
+
+const TextStyle _kCupertinoActionSheetActionTextStyle = const TextStyle(


You don't really need to have such a long name here, since it's local to this file: _kActionTextStyle would be fine.

gspencergoog · 2018-07-11T00:11:33Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+
+    void _onTap() {
+      widget.onPressed;
+      Navigator.pop(context); //SHOULD THE ACTION SHEET BE HANDLING THIS?


No, the onPressed will handle that. Look at how the dialog demo in the gallery does it. In order to do it here, you'd have to know what value to pass back.

gspencergoog · 2018-07-11T00:11:48Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+    }
+
+    void _onTap() {
+      widget.onPressed;


Should be widget.onPressed(); (or it won't actually call the function).

gspencergoog · 2018-07-11T00:20:34Z

packages/flutter/lib/src/cupertino/route.dart

+  AnimationController createAnimationController() {
+    assert(_animationController == null);
+    _animationController = new AnimationController(
+      duration: const Duration(milliseconds: 500),


This feels too long to me, but if that's what iOS does, then it's fine.

Yes, you are correct, was waiting to play with duration and curve until I got the positioning of the sheet correct (should have made a note of this)!

gspencergoog · 2018-07-11T00:55:20Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+
+    return Padding(
+      padding: const EdgeInsets.symmetric(horizontal: 8.0),
+      child: new Center(


Here's your problem with getting the child size in your layout's getPositionForChild. The Center is expanding to fill the whole available area. Just remove this Center.

Perfect, that worked! Do you think making that change will help with my sizing problem if I continue to play around with constraints?

Yes, I think it should. Give it a try.

xster

I took a quick look at the route.dart part

xster · 2018-07-13T23:59:39Z

packages/flutter/lib/src/cupertino/route.dart

+}) {
+  return Navigator.push(context, new _CupertinoModalPopupRoute<T>(
+    builder: builder,
+    barrierLabel: 'ModalPopup',


This is the announced label for what happens when you tap the barrier. Probably 'dismiss' or something.

xster · 2018-07-14T00:01:02Z

packages/flutter/lib/src/cupertino/route.dart

+  }
+}
+
+class _ModalBottomSheet<T> extends StatefulWidget {


Does this need to be Stateful? Can we just collapse this whole thing into _CupertinoModalPopupRoute and avoid a layer of abstraction?

xster · 2018-07-14T00:06:45Z

packages/flutter/lib/src/cupertino/route.dart

+  Widget build(BuildContext context) {
+    return new AnimatedBuilder(
+        animation: widget.route.animation,
+        builder: (BuildContext context, Widget child) {


Building a AnimatedBuilder without using a child is fairly expensive (since the child tree itself needs to rebuild each frame even though the action sheets themselves presumably don't change during the animation).

This setup seems complex. Can't it just be a FractionalTranslation with a child?

nataliesampsell · 2018-08-07T00:54:59Z

@gspencergoog and @xster I've pushed my latest work on CupertinoActionSheet for your review. Matt requested that I copy any code I was using from his dialog PR (as opposed to me trying to land my work on top of his), so there is overlap between my PR and his - just so you're aware if you are also reviewing his PR.

gspencergoog · 2018-08-07T01:01:33Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+  textBaseline: TextBaseline.alphabetic,
+);
+
+// _kCupertinoAlertBlurOverlayDecoration is applied to the blurred backdrop to


nit: Instead of saying _kCupertinoAlertBlurOverlayDecoration, just say "This decoration". That way, if the variable is renamed using a refactoring tool, the name won't be out of sync.

gspencergoog · 2018-08-07T01:02:22Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+
+/// An iOS-style action sheet.
+///
+/// An action sheet is a specific style of alert that present the user


present --> presents

gspencergoog · 2018-08-07T01:02:54Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+/// sheet title and message text style.
+///
+/// To display action buttons that look like standard iOS action sheet buttons,
+/// provide [ActionSheetAction]s for the [actions] given to this action shet.


shet --> sheet

gspencergoog · 2018-08-07T01:04:17Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+  }
+}
+
+/// A button typically used in an [CupertinoActionSheet].


an --> a
(or should this still be ActionSheet instead of CupertinoActionSheet?)

Yes, apologies for all of the ActionSheet/CupertinoActionSheet mixups. I believe it should be CupertinoActionSheet, in keeping with the Cupertino naming convention, but I started with it being ActionSheet and forgot to make the change throughout the comments.

gspencergoog · 2018-08-07T01:06:12Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+  }
+}
+
+// iOS style layout policy for sizing an alert's content section and action


iOS style --> An iOS-style

gspencergoog · 2018-08-07T01:14:57Z

packages/flutter/lib/src/cupertino/route.dart

+///
+/// See also:
+///
+///  * [ActionSheet], which is the widget usually returned by the `builder`


Should ActionSheet be CupertinoActionSheet? Or is that all happening in the other rename PR?

gspencergoog · 2018-08-07T01:15:57Z

packages/flutter/lib/src/painting/box_decoration.dart


+  /// The blend mode applied to the [color] or [gradient] background of the box.
+  ///
+  /// If no [backgroundBlendMode] is provided then the default painting blend


provided --> provided,
(add comma)

gspencergoog · 2018-08-07T01:16:10Z

packages/flutter/lib/src/painting/box_decoration.dart

+  /// If no [backgroundBlendMode] is provided then the default painting blend
+  /// mode is used.
+  ///
+  /// If no [color] or [gradient] is provided then blend mode has no impact.


provided --> provided,
(add comma)

gspencergoog · 2018-08-07T01:17:34Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+///
+/// See also:
+///
+///  * [CupertinoActionSheet], an alert that presents the user with a set of two or


Should this be ActionSheet instead of CupertinoActionSheet?

gspencergoog · 2018-08-07T01:18:18Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+///
+///  * [CupertinoActionSheet], an alert that presents the user with a set of two or
+///    more choices related to the current context.
+class ActionSheetAction extends StatelessWidget {


Should this be CupertinoActinoSheetAction?

xster

Very nice. Just a few small comments.

xster · 2018-08-07T08:30:57Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+
+  Widget _buildCancelButton() {
+    final double cancelPadding = (actions != null || message != null || title != null)
+        ? _kEdgeHorizontalPadding : 0.0;


You're reading a thing called horizontal padding but you're only using it vertically it seems.

xster · 2018-08-07T08:33:21Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+  Widget build(BuildContext context) {
+    final List<Widget> children = <Widget>[
+      new Flexible(child: ClipRRect(
+        borderRadius: BorderRadius.circular(12.0),


Don't use dart2 syntax yet

xster · 2018-08-07T08:34:24Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+  }
+}
+
+class _CupertinoAlertRenderWidget extends RenderObjectWidget {


I'm assuming everything here below are things copied from @matthew-carroll's PR?

Yes everything below is copied, with some modifications to make things work for action sheets specifically

xster · 2018-08-07T08:38:17Z

packages/flutter/test/cupertino/action_sheet_test.dart

+}
+
+Widget createAppWithButtonThatLaunchesActionSheet(Widget actionSheet) {
+  return new MaterialApp(


Avoid using Material widgets here. Just use their Cupertino equivalents

xster · 2018-08-07T08:42:50Z

packages/flutter/test/cupertino/action_sheet_test.dart

+      ),
+    );
+
+    final DefaultTextStyle widget = tester.widget(find.byType(DefaultTextStyle));


find.widgetWithText to be more precise

xster · 2018-08-07T08:44:40Z

packages/flutter/test/cupertino/action_sheet_test.dart

+    await tester.pumpWidget(
+      createAppWithButtonThatLaunchesActionSheet(
+          new CupertinoActionSheet(
+                actions: <Widget>[


indents are a bit off here and above

xster · 2018-08-07T08:45:25Z

packages/flutter/test/cupertino/action_sheet_test.dart

+    await tester.pump(const Duration(seconds: 1));
+
+    // Check that the title/message section is not displayed
+    expect(actionScrollController.offset, 0.0);


Not sure how the actions scroll controller's offset is indicative of the presence or absence of the title/message section

xster · 2018-08-07T08:45:32Z

packages/flutter/test/cupertino/action_sheet_test.dart

+
+    // Check that the button's vertical size is the same.
+    expect(tester.getSize(find.widgetWithText(CupertinoActionSheetAction, 'One')).height,
+        equals(tester.getSize(find.widgetWithText(CupertinoActionSheetAction, 'Two')).height));


nit: indent

xster · 2018-08-07T08:45:43Z

packages/flutter/test/cupertino/action_sheet_test.dart

+    expect(actionScrollController.offset, 0.0);
+
+    // Check that the button's vertical size is the same.
+    expect(tester.getSize(find.widgetWithText(CupertinoActionSheetAction, 'One')).height,


Perhaps check their exact coordinates?

xster · 2018-08-07T08:46:46Z

packages/flutter/test/cupertino/action_sheet_test.dart

+    await tester.tap(find.text('Go'));
+    await tester.pump();
+
+    expect(tester.getSize(find.byType(CupertinoActionSheet)).height, 132.33333333333334);


moreOrLessEquals (or it can be undeterministic depending on the machine it's running on)

xster

LGTM

xster · 2018-08-09T22:39:06Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+    this.messageScrollController,
+    this.actionScrollController,
+    this.cancelButton,
+  })  : assert(actions != null || title != null || message != null || cancelButton != null),


Add an error message

xster · 2018-08-09T22:43:21Z

packages/flutter/lib/src/cupertino/action_sheet.dart

+
+    if (cancelButton != null) {
+      children.add(
+        _buildCancelButton(),


Is there no blur behind the cancel button?

No, from observation/experimentation I've found that the cancel button isn't transparent like the other buttons.

Ah cool, thanks for testing

xster · 2018-08-09T22:50:25Z

packages/flutter/lib/src/cupertino/route.dart

+  @override
+  Widget buildTransitions(BuildContext context, Animation<double> animation, Animation<double> secondaryAnimation, Widget child) {
+    if (_animation.value == 1.0) {
+      context.findRenderObject().markNeedsSemanticsUpdate();


buildTransition isn't guaranteed to be called each time the animation value changes.

I've found in practice that the animation is long enough and the curve is such that buildTransitions ends up being called several times while the animation value is 1.0. I understand the concern though - could the markNeedsSemanticsUpdate() be called in buildPage, or do you know of another way to tell the render object there needs to be a semantics update when the animation is complete?

What about dismissal? Don't you want to call it as soon as it's not 1.0 anymore? Add animation status listeners on createAnimation. (Make sure to unregister the listener when it's done though)

actually why do you call this here in the route itself? What is the issue you're solving?

I was thinking I'd want to call it when the animation was complete so the semantic bounds could be placed correctly? Does that make sense?

Oh, just saw your followup question - the issue is that the semantic bounds for the action sheet end up being drawn offscreen, so I need to have the semantics update once the action sheets get in their final position.

I see. I would leave this out altogether. If it's easy to produce a reduced test case that demonstrates that the semantic box is at the wrong place inside a fractional translation, this should be fixed in the semantics pipeline itself.

I'd leave it broken and leave an issue describing the misbehavior.

xster · 2018-08-09T22:51:37Z

packages/flutter/lib/src/cupertino/route.dart

+    return new Align(
+      alignment: Alignment.bottomCenter,
+      child: new FractionalTranslation(
+        translation: new Tween<Offset>(


Create this once in createAnimation

nsampsell added 7 commits June 20, 2018 17:46

Added necessary cupertino file + update cupertino.dart

7af93e9

Merge remote-tracking branch 'upstream/master' into action_sheets

1c34e27

Initial work on action sheets

2b6bcb8

Added ActionSheetAction class

72f4876

Layout for action sheet and begin work on popup mechanism

5a5ca94

Trying out layout ideas

63daa71

Merge branch 'action_sheets' of github.com:nataliesampsell/flutter in…

6f7df32

…to action_sheets

googlebot added the cla: yes label Jul 10, 2018

gspencergoog reviewed Jul 11, 2018

View reviewed changes

xster reviewed Jul 14, 2018

View reviewed changes

nsampsell added 7 commits July 16, 2018 10:37

Further work on layout and popup mechanism

326a59d

Fixed popup mechanism curve and barrier color

3e8bad8

Merge remote-tracking branch 'upstream/master' into action_sheets

f8e2013

Continuing work on action sheets

4822fca

Cleanup code

63270d9

Merge remote-tracking branch 'upstream/master' into action_sheets

7736e01

Revert unnecessary formatting changes

b0a60c5

nataliesampsell changed the title ~~[WIP] Action sheets~~ CupertinoActionSheet Aug 7, 2018

Removed const keyword

5e44c5b

gspencergoog requested changes Aug 7, 2018

View reviewed changes

Fixes to comments and modal_barrier tests

6421911

xster reviewed Aug 7, 2018

View reviewed changes

nsampsell added 2 commits August 7, 2018 11:13

Action sheet fixes

666f29f

Fix modal_barrier semantics

12d6846

gspencergoog approved these changes Aug 8, 2018

View reviewed changes

xster approved these changes Aug 9, 2018

View reviewed changes

xster mentioned this pull request Aug 9, 2018

Implement CupertinoActionSheet #7376

Closed

nsampsell added 2 commits August 9, 2018 21:07

Final fixes

3df69f9

Merge remote-tracking branch 'upstream/master' into action_sheets

f4eb6cb

nataliesampsell merged commit 96326d4 into flutter:master Aug 10, 2018

nataliesampsell deleted the action_sheets branch August 10, 2018 04:55

github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021

CupertinoActionSheet #19232

CupertinoActionSheet #19232

Uh oh!

Conversation

nataliesampsell commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nataliesampsell commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nataliesampsell commented Jul 10, 2018 •

edited

Loading

nataliesampsell commented Aug 7, 2018 •

edited

Loading