Drawer fix by jslavitz · Pull Request #20015 · flutter/flutter

jslavitz · 2018-07-30T23:54:40Z

Fixes #19704

jonahwilliams

Just a thought - maybe this is worth doing to avoid the assert. What if instead of not building one drawer when the other is opened - you changed the order in which they were built such that the open drawer is on top?

Might be a simple change that will still fix the issue without requiring an assert.

jonahwilliams · 2018-07-31T00:06:09Z

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


 import 'package:flutter/foundation.dart';
 import 'package:flutter/widgets.dart';
+import 'package:flutter/material.dart';


import the appropriate source files from material (like below) instead of the entire lib. The circular import will break the docs shard

jonahwilliams · 2018-07-31T00:07:23Z

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

  /// Typically called by [ScaffoldState.openDrawer].
  void open() {
    _controller.fling(velocity: 1.0);
+    if(widget.drawerCallback != null)


space between if and (, here and elsewhere

jonahwilliams · 2018-07-31T00:07:44Z

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

-          behavior: HitTestBehavior.translucent,
-          excludeFromSemantics: true,
-          child: new Container(width: _kEdgeDragWidth)
+            key: _gestureDetectorKey,


Revert unrelated change, here and elsewhere

jonahwilliams · 2018-07-31T00:08:21Z

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

+
+  void _drawerOpenedCallback(bool isOpened) {
+    _drawerOpened = isOpened;
+    setState(() {});


For more idiomatic setState:

setState(() { _drawerOpened = isOpened; });

jonahwilliams · 2018-07-31T00:09:19Z

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

-        removeBottomPadding: false,
-      );
+    if(!_endDrawerOpened) {
+      if (widget.drawer != null) {


Combine these if statements -
if (widget.drawer != null && !_endDrawerOpened)

jonahwilliams · 2018-07-31T00:09:29Z

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

-        removeRightPadding: textDirection == TextDirection.rtl,
-        removeBottomPadding: false,
-      );
+    if (!_drawerOpened) {


jonahwilliams · 2018-07-31T00:10:43Z

packages/flutter/test/material/scaffold_test.dart

+            right: false,
+            bottom: false,
+            child: new Scaffold(
+                endDrawer: new Drawer(


ident by 2 lines

jonahwilliams · 2018-07-31T01:59:34Z

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

 import 'package:flutter/foundation.dart';
 import 'package:flutter/widgets.dart';

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


Don't forget to fix this import 📁

But I get like 100 errors if I take it out... How do I know which specific files to include?

On the auto import suggestion in intellij you should see an option that imports from '/material/src` - just pick those and then make them relative.

Looks like I didn't need that import, must have not been filtering for the errors in the current file before.

Fixed the issues!

…erFix2 Trying to fix stupid errors

jonahwilliams · 2018-08-01T00:22:26Z

Ignore the result of travis BTW

jonahwilliams · 2018-08-01T00:52:21Z

@jslavitz This is good to submit

Hixie · 2018-08-03T19:59:51Z

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

    GlobalKey key,
    @required this.child,
    @required this.alignment,
+    this.drawerCallback,


this should be called onFoo, not fooCallback

Hixie · 2018-08-03T20:00:32Z

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

  final DrawerAlignment alignment;

+  /// Optional callback that is called when a [Drawer] is opened or closed.
+  final DrawerCallback drawerCallback;


maybe onDrawerChange or some such.

Or maybe even better, have two callbacks, onOpen and onClose.

Hixie · 2018-08-03T20:03:12Z

packages/flutter/test/material/scaffold_test.dart

      numNotificationsAtLastFrame = listenerState.numNotifications;
    });

+    testWidgets('Simultaneous drawers on either side', (WidgetTester tester) async {


why was this moved to the ScaffoldGeometry test section?

Hixie · 2018-08-03T20:03:42Z

This seems to have regressed the stock_animation_close_first_frame_average benchmark a little.

jslavitz · 2018-08-03T20:58:43Z

Where can I check out that benchmark? Maybe we could just do the set state using Future so that it doesn't get called on the first frame of the animation?

Hixie · 2018-08-03T23:57:08Z

I don't really understand what caused the regression. Nothing here looks like it should be slow.

https://flutter-dashboard.appspot.com/benchmarks.html has the results of the benchmarks
The benchmarks themselves are in dev/devicelab, there's a README there that explains how to run them locally.

jslavitz · 2018-08-04T00:01:07Z

So what benchmark am I looking at? The only thing I can figure is that the set state calls slowed it down? Outside of that, I agree, nothing should be causing performance issues.

jonahwilliams · 2018-08-04T00:10:05Z

@jslavitz lets look at them on Monday

Hixie · 2018-08-10T00:39:40Z

Did y'all come to any conclusion on this?

Drawer fix

75e322f

googlebot added the cla: yes label Jul 30, 2018

jonahwilliams previously requested changes Jul 31, 2018

View reviewed changes

jslavitz added 3 commits July 30, 2018 18:14

fixed nits

95dd78b

fixed nits

27247c1

fixed nits

48d19a2

jonahwilliams reviewed Jul 31, 2018

View reviewed changes

final change

34f612f

jonahwilliams requested a review from gspencergoog July 31, 2018 16:44

jonahwilliams approved these changes Jul 31, 2018

View reviewed changes

Drawer fix final

13a61b3

jonahwilliams removed the request for review from gspencergoog July 31, 2018 17:48

Merge branch 'master' of https://github.com/flutter/flutter into draw…

033ee4a

…erFix2 Trying to fix stupid errors

jslavitz merged commit d2ab29d into flutter:master Aug 1, 2018

jslavitz deleted the drawerFix2 branch August 1, 2018 17:14

Hixie reviewed Aug 3, 2018

View reviewed changes

Jon-Salmon mentioned this pull request Aug 14, 2018

Drawer Open/Close callback #20581

Closed

blac3777 mentioned this pull request Jun 10, 2019

The drawer can be dragged underneath the endDrawer #34151

Closed

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

Conversation

jslavitz commented Jul 30, 2018

Uh oh!

jonahwilliams left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jonahwilliams commented Aug 1, 2018

Uh oh!

jonahwilliams commented Aug 1, 2018

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 Aug 3, 2018

Uh oh!

jslavitz commented Aug 3, 2018

Uh oh!

Hixie commented Aug 3, 2018

Uh oh!

jslavitz commented Aug 4, 2018

Uh oh!

jonahwilliams commented Aug 4, 2018

Uh oh!

Hixie commented Aug 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonahwilliams left a comment •

edited

Loading