Material scaffold to have simultaneous left-and-right drawers by 5u3it · Pull Request #12686 · flutter/flutter

5u3it · 2017-10-24T07:36:07Z

Material scaffold to have simultaneous left and right drawers

Hixie · 2017-10-24T16:28:38Z

examples/flutter_gallery/ios/Runner/Assets.xcassets/AppIcon.appiconset/Contents.json

    "author" : "xcode"
  }
-}
+}


looks like the IDE made some unexpected changes here

Hixie · 2017-10-24T17:32:00Z

Some high-level comments:

rather than "left drawer" and "right drawer" we should probably call these "drawer" and "end-side drawer". In right-to-left environments (e.g. Arabic, Hebrew) the "left" drawer ends up on the right and the "right" drawer ends up on the left, so rather than call them "left" and "right" we normally refer to the sides as "start" and "end".
rather than rename the left (start) drawer, we should leave it named drawer so that we don't break existing applications.
since we need to be agnostic about the side anyway (start vs end), we can just have a single controller class that handles both sides (like it does today, in fact), and instantiate it with a different argument for the "right" (end) side.

Hixie · 2017-10-24T17:34:03Z

We'll need some tests, too.

5u3it · 2017-10-25T07:04:49Z

@Hixie Thanks for the feedback. Following points have been incorporated:

rather than "left drawer" and "right drawer" we should probably call these "drawer" and "end-side drawer". In right-to-left environments (e.g. Arabic, Hebrew) the "left" drawer ends up on the right and the "right" drawer ends up on the left, so rather than call them "left" and "right" we normally refer to the sides as "start" and "end": DONE
rather than rename the left (start) drawer, we should leave it named drawer so that we don't break existing applications: DONE
since we need to be agnostic about the side anyway (start vs end), we can just have a single controller class that handles both sides (like it does today, in fact), and instantiate it with a different argument for the "right" (end) side: DONE

abarth · 2017-10-25T18:07:25Z

packages/flutter/lib/src/material/app_bar.dart

-  void _handleDrawerButton() {
-    Scaffold.of(context).openDrawer();
+  void _handleDrawerButtonLeft() {
+    Scaffold.of(context).openLeftDrawer();


These two functions still refer to the "left" drawer, which isn't an accurate name in languages with right-to-left reading orders.

abarth · 2017-10-25T18:08:31Z

packages/flutter/lib/src/material/app_bar.dart

+        icon: const Icon(Icons.menu),
+        onPressed: _handleDrawerButtonEndSide,
+        tooltip: MaterialLocalizations.of(context).openAppDrawerTooltip,
+      );      


Does the mean the button to open the end-side drawer appears only when there are no other actions in the app bar?

Yes, that is the assumption.

abarth · 2017-10-25T18:10:04Z

packages/flutter/lib/src/material/drawer.dart

 import 'material.dart';
+import 'scaffold.dart';
+
+enum DrawerType { START, END }


s/START/start/
s/END/end/

I wonder if DrawerAlignment would be a better name for this enum. You could imagine DrawerType as something more general (e.g., floating, expanding, circular).

abarth · 2017-10-25T18:12:42Z

packages/flutter/lib/src/material/drawer.dart

+        return;
+      } else {
+        close();
+      } 


DrawerController already knows how to flip around based on the reading direction. Rather than plumb this logic in this way, maybe it would make sense to flip the drawer controller around in the same way.

Specifically, I'd recommend keeping _controller.value logically "closed" and just change the visual presentation of the logical state. That way _controller.isDismissed still corresponds to the drawer being closed, which makes more sense semantically.

abarth · 2017-10-25T18:13:29Z

packages/flutter/lib/src/material/drawer.dart

-        _controller.value += delta;
-        break;
+    if (widget.type == DrawerType.START) {
+      if (Scaffold.of(context).isEndSideDrawerOpen() == true) {


There should be no need to interact with the Scaffold here. The DrawerController should not have any knowledge of the Scaffold and should work independently of it.

abarth · 2017-10-25T18:14:51Z

packages/flutter/lib/src/material/drawer.dart

-      case TextDirection.ltr:
-        _controller.value += delta;
-        break;
+    if (widget.type == DrawerType.START) {


Rather than break down these case by DrawerAlignment. We should translate the visual delta into a logical valueDelta by (possibly) negating it once for DrawerAlignment and (possibly) again for Directionality. Then we can apply the valueDelta to _controller.value on a single line of code.

abarth · 2017-10-25T18:16:06Z

packages/flutter/lib/src/material/drawer.dart

-
-    } else if (_controller.value < 0.5) {
-      close();
+    if (widget.type == DrawerType.START) {


Same comment here. Rather than duplicating all this code, we can translate visualVelocity into valueVelocity by first considering the DrawerAlignment and then considering the Directionality. Once we do that, we can fling the _controller using the commuted valueVelocity.

abarth · 2017-10-25T18:17:55Z

packages/flutter/lib/src/material/drawer.dart

    _controller.fling(velocity: -1.0);
  }

+  bool isOpen() {


This function doesn't appear to be needed.

abarth · 2017-10-25T18:18:30Z

packages/flutter/lib/src/material/drawer.dart

      return new Align(
-        alignment: AlignmentDirectional.centerStart,
+        alignment: (widget.type == DrawerType.START) ? 
+          AlignmentDirectional.centerStart : AlignmentDirectional.centerEnd,


I'd break these out into private getters and use a switch statement instead of operator== on the enum.

abarth · 2017-10-25T18:19:15Z

packages/flutter/lib/src/material/scaffold.dart

+  /// [ScaffoldState.openDrawer] function.
+  ///
+  /// Typically a [Drawer].
+  final Widget endSideDrawer;


What does side mean in this context? A better name might be endDrawer

abarth · 2017-10-25T18:20:24Z

packages/flutter/lib/src/material/scaffold.dart

  ///
  /// See [Scaffold.of] for information about how to obtain the [ScaffoldState].
-  void openDrawer() {
+  void opendrawer() {


s/opendrawer/openDrawer/

abarth · 2017-10-25T18:22:24Z

packages/flutter/lib/src/material/scaffold.dart

+    } else {
+      return false;
+    }
+  }


It looks like you're using these functions to couple the behavior of the start and end drawers. The way you've done this introduces too tight a coupling. I'm not clear on what behavior you're trying to create. Can you explain what end-user visible behavior you're trying to achieve?

The idea was to have only one of the drawers open at a time. Otherwise the drawers will overlap on top of one another.

That makes sense, but there are a bunch of different interaction designs that might achieve that goal. For example, you could have an edge-trigger on each drawer's animation leaving the isDismissed state that triggers the other drawer to close() (and cancels any current gestures interacting with that drawer).

Sure. I'll check up on this. Thanks a lot for the feedback.

abarth · 2017-10-25T18:23:34Z

We'll also need tests, including ones that exercise the interaction between start and end drawers as well as covering the LTR and RTL cases.

abarth · 2017-10-27T03:51:11Z

packages/flutter/lib/src/material/drawer.dart

 import 'list_tile.dart';
 import 'material.dart';

+enum DrawerAlignment { START, END }


s/START/start/
s/END/end/

We use camelCase for enum values. Also, please add dartdoc for all public identifiers.

abarth · 2017-10-28T05:59:03Z

packages/flutter/lib/src/material/drawer.dart

  ///
  /// Typically a [Drawer].
  final Widget child;
+  final DrawerAlignment type;


missing dartdoc

Also, should probably be named "alignment" now that the type is called DrawerAlignment rather than DrawerType

abarth · 2017-10-28T06:01:19Z

packages/flutter/lib/src/material/drawer.dart

+      if (canPop) {
+        Navigator.of(context).pop();
+        return;
+      }


I don't think this interaction design is of sufficient quality to land. We can land the patch without an interaction design for the two-drawer case or we can figure out an interaction design for the two-drawer case in this patch, but I'd prefer not to land a patch with a half-designed interaction.

Okay.
I have not been able to get the behavior working via addStatusListener on the _controller.
Hence, I'm removing this logic from _move for now.

abarth · 2017-10-28T06:02:11Z

packages/flutter/lib/src/material/drawer.dart

+      case DrawerAlignment.end:
+        delta = -delta;
+        break;
+    }


I'm surprised the analyzer doesn't complain about the start case not being handled. In any case, the idiom we prefer is to handle the start case (just break; is fine).

abarth · 2017-10-28T06:02:20Z

packages/flutter/lib/src/material/drawer.dart

+        case DrawerAlignment.end:
+          visualVelocity = -visualVelocity;
+          break;
+      }


abarth · 2017-10-28T06:03:34Z

packages/flutter/lib/src/material/scaffold.dart

-  /// devices.
-  ///
+  /// devices. Swipes in from either left-to-right (LTR languages) or
+  /// right-to-left (RTL languages)


This is a tiny nit, but I'd replace "LTR languages" with "[TextDirection.ltr]" (and the same for "RTL languages") so that the dartdocs link back to the TextDirection docs, which explain more about what's going on here.

abarth · 2017-10-28T06:03:52Z

packages/flutter/lib/src/material/scaffold.dart


  /// Whether this scaffold has a non-null [Scaffold.drawer].
  bool get hasDrawer => widget.drawer != null;
+  bool get hasEndDrawer => widget.endDrawer != null;


Missing dartdoc

5u3it · 2017-11-01T06:39:46Z

@abarth Do you have any more feedback?

abarth · 2017-11-01T16:28:14Z

packages/flutter/lib/src/material/drawer.dart

    @required this.child,
-  }) : assert(child != null),
+    @required this.alignment,
+  }) : assert(child != null || alignment != null),


Do you really mean || here? I would have expected

assert(child != null), assert(alignment != null),

Which is the "and" rather than "or" of these conditions.

abarth · 2017-11-01T16:30:58Z

packages/flutter/lib/src/material/drawer.dart

  final ColorTween _color = new ColorTween(begin: Colors.transparent, end: Colors.black54);
  final GlobalKey _gestureDetectorKey = new GlobalKey();

+  AlignmentDirectional get _drawerStartAlignment {


The term "start" here is a bit confusing because "start" and "end" are possible alignment values themselves. For example, it's confusing that a "start alignment" would return "centerEnd".

Maybe _drawerOuterAlignment for this one and _drawerInnerAlignment for the one below?

abarth · 2017-11-01T16:32:02Z

Just a couple minor changes and we can land this patch. Thanks so much for sticking with it!

5u3it · 2017-11-02T03:18:36Z

@abarth Thanks a lot for your feedback. Your latest feedback has been incorporated.

abarth · 2017-11-02T03:41:56Z

examples/flutter_gallery/ios/Runner.xcodeproj/project.pbxproj

 	};
 	rootObject = 97C146E61CF9000F007C117D /* Project object */;
-}
+}


You've still got spurious changes to these two files.

abarth · 2017-11-02T03:42:57Z

packages/flutter/lib/src/material/scaffold.dart

@@ -1086,14 +1143,16 @@ class PersistentBottomSheetController<T> extends ScaffoldFeatureController<_Pers
 class _ScaffoldScope extends InheritedWidget {


These changes to _ScaffoldScope aren't needed anymore.

Reverted the changes

abarth · 2017-11-02T03:43:24Z

Sorry I missed that the changes to _ScaffoldScope aren't need anymore in my last review :(

5u3it · 2017-11-02T04:17:47Z

@abarth Please could be clarify what you mean by "_ScaffoldScope changes aren't needed"?

abarth · 2017-11-02T04:21:51Z

Please could be clarify what you mean by "_ScaffoldScope changes aren't needed"?

You've add an hasEndDrawer property to _ScaffoldScope, but that property isn't used for anything. You can undo your changes to _ScaffoldScope and everything will continue to work the same way.

5u3it · 2017-11-02T04:27:46Z

@abarth Reverted the _ScaffoldScope changes

abarth · 2017-11-02T04:34:46Z

Yay! Thanks so much for the patch and all the revisions.

5u3it · 2017-11-02T04:36:37Z

Thanks for working alongside :)

…r#12686) Adds `Scaffold#endDrawer` property to supply a second drawer to a Scaffold.

lsi8 · 2019-12-10T00:47:34Z

Please give issues #39736 & #34151 some attention ):

Material scaffold to have simultaneous left-and-right drawers

3774061

googlebot added the cla: yes label Oct 24, 2017

Example changes reverted

362ae0d

Hixie reviewed Oct 24, 2017

View reviewed changes

5u3it added 2 commits October 25, 2017 12:28

Incorporated feedback received from Hixie

d7e316f

Merge branch 'master' of github.com:flutter/flutter into chq_scaffold

3cf457b

sethladd added framework flutter/packages/flutter repository. See also f: labels. c: new feature Nothing broken; request for a new capability labels Oct 25, 2017

sethladd mentioned this pull request Oct 25, 2017

Provision for simultaneous left and right navigation drawers absent in Scaffold widget #11388

Closed

abarth reviewed Oct 25, 2017

View reviewed changes

5u3it added 4 commits October 26, 2017 11:31

Adam's (abarth) feedback incorporated

74585a2

Fixed overlapping of drawers

bb620e1

Getters for drawer alignment

37d8645

Simple test to validate simultaneous drawers

2e0beab

abarth reviewed Oct 27, 2017

View reviewed changes

abarth reviewed Oct 28, 2017

View reviewed changes

Adam's (abarth) feedback4 incorporated

53a6738

abarth reviewed Nov 1, 2017

View reviewed changes

Adam's (abarth) feedback5 incorporated

768a239

abarth reviewed Nov 2, 2017

View reviewed changes

5u3it added 2 commits November 2, 2017 09:43

Reverted _ScaffoldScope changes. Spurious changes removal.

0a543a8

_ScaffoldScope changes

3379fca

Reverted _ScaffoldScope changes

68dc653

abarth merged commit c608e66 into flutter:master Nov 2, 2017

abarth mentioned this pull request Nov 2, 2017

Fix analyzer warnings #12840

Merged

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018

Material scaffold to have simultaneous left-and-right drawers (flutte…

6ff4fd1

…r#12686) Adds `Scaffold#endDrawer` property to supply a second drawer to a Scaffold.

blac3777 mentioned this pull request Jul 24, 2018

The endDrawer can be dragged over the drawer #19704

Closed

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

		@@ -1086,14 +1143,16 @@ class PersistentBottomSheetController<T> extends ScaffoldFeatureController<_Pers
		class _ScaffoldScope extends InheritedWidget {

Conversation

5u3it commented Oct 24, 2017

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!

Hixie commented Oct 24, 2017

Uh oh!

Hixie commented Oct 24, 2017

Uh oh!

5u3it commented Oct 25, 2017

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!

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!

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!

abarth commented Oct 25, 2017

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