Surface border parameter in CupertinoSliverNavigationBar by edman · Pull Request #18194 · flutter/flutter

edman · 2018-06-05T09:01:51Z

xster · 2018-06-05T20:03:07Z

Thanks for your contribution. Please add a test to nav_bar_test.dart. There's an example in there already called 'Border should not be displayed when null' that you can adapt for CupertinoSliverNavigationBar

edman · 2018-06-05T23:28:05Z

@xster tests added. Thanks for the pointers.

xster

Thanks for adding the test!

Some minor feedback

xster · 2018-06-05T23:32:52Z

+  testWidgets(
+      'Border is displayed by default in sliver nav bar',
+      (WidgetTester tester) async {
+    final ScrollController scrollController = new ScrollController();


since you're not using any of these in the test, you don't have to define any of these variables

xster · 2018-06-05T23:33:04Z

+                  controller: scrollController,
+                  slivers: <Widget>[
+                    new CupertinoSliverNavigationBar(
+                      leading: new Placeholder(key: leadingKey),


you can just keep the largeTitle and remove the rest

Done.

I did so at first, but left only the titleKey and was getting an error about the same global key being used in multiple widgets, which was fixed with the additional keys. I don't fully understand what's going on but trying now again I see the error also goes away removing all key definitions.

xster · 2018-06-05T23:36:27Z

+      ),
+    );
+
+    final DecoratedBox decoratedBox =


Ah, sorry, the existing test wasn't ideal actually since it's not very explicit.

Replace with

tester.widget(find.descendant( of: find.byType(CupertinoSliverNavigationBar), matching: find.byType(DecoratedBox), ));

instead which says get the first DecoratedBox under CupertinoSliverNavigationBar instead of get the third DecoratedBox anywhere in the whole app.

Done.

This is nice, I found elementAt(2) so ugly as I was writing it. Also went on and updated other tests in the file.

xster · 2018-06-05T23:36:52Z

+  testWidgets(
+      'Border is not displayed when null in sliver nav bar',
+      (WidgetTester tester) async {
+    final ScrollController scrollController = new ScrollController();


ditto, you can remove all this

xster · 2018-06-05T23:37:06Z

+                  controller: scrollController,
+                  slivers: <Widget>[
+                    new CupertinoSliverNavigationBar(
+                      leading: new Placeholder(key: leadingKey),


same here. Just keep the largeTitle and border

xster · 2018-06-05T23:37:19Z

+  testWidgets(
+      'Border can be overridden in sliver nav bar',
+      (WidgetTester tester) async {
+    final ScrollController scrollController = new ScrollController();


xster · 2018-06-05T23:38:12Z

    expect(tester.getBottomLeft(find.text('Title')).dy, 44.0 - 8.0); // Extension gone, (static part - padding) left.
  });

+  testWidgets(


can we move this below 'Border should not be displayed when null' to keep things in order?

Done. Moved new tests below the existing CupertinoNavigationBar tests.

edman

@xster comments resolved.

edman · 2018-06-06T09:00:39Z

+                  controller: scrollController,
+                  slivers: <Widget>[
+                    new CupertinoSliverNavigationBar(
+                      leading: new Placeholder(key: leadingKey),


Done.

I did so at first, but left only the titleKey and was getting an error about the same global key being used in multiple widgets, which was fixed with the additional keys. I don't fully understand what's going on but trying now again I see the error also goes away removing all key definitions.

edman · 2018-06-06T09:31:27Z

+      ),
+    );
+
+    final DecoratedBox decoratedBox =


Done.

This is nice, I found elementAt(2) so ugly as I was writing it. Also went on and updated other tests in the file.

edman · 2018-06-06T09:52:53Z

    expect(tester.getBottomLeft(find.text('Title')).dy, 44.0 - 8.0); // Extension gone, (static part - padding) left.
  });

+  testWidgets(


Done. Moved new tests below the existing CupertinoNavigationBar tests.

edman · 2018-06-06T09:53:01Z

+  testWidgets(
+      'Border is displayed by default in sliver nav bar',
+      (WidgetTester tester) async {
+    final ScrollController scrollController = new ScrollController();


edman · 2018-06-06T09:53:47Z

+  testWidgets(
+      'Border is not displayed when null in sliver nav bar',
+      (WidgetTester tester) async {
+    final ScrollController scrollController = new ScrollController();


edman · 2018-06-06T09:53:53Z

+                  controller: scrollController,
+                  slivers: <Widget>[
+                    new CupertinoSliverNavigationBar(
+                      leading: new Placeholder(key: leadingKey),


edman · 2018-06-06T09:54:10Z

+  testWidgets(
+      'Border can be overridden in sliver nav bar',
+      (WidgetTester tester) async {
+    final ScrollController scrollController = new ScrollController();


xster

Super awesome. Thanks for the PR! Will merge it in when the build is green.

xster · 2018-06-06T18:29:12Z

    );

-    final DecoratedBox decoratedBox = tester.widgetList(find.byType(DecoratedBox)).elementAt(1);
+    final DecoratedBox decoratedBox = tester.widgetList(find.descendant(


Thanks for fixing these too :)

7e205b3 Roll src/fuchsia/sdk/mac from 1MVsE... to 4MCVP... (flutter#18217) 9193d8f Roll src/third_party/skia 0dc207f836da..a14084ba1b41 (3 commits) (flutter#18219) 403931f Add FlValue (flutter#18185) 46a6d96 [SkParagraph] Copy text height behavior to the Skia paragraph style (flutter#18178) e7ee47d [web] Implement matrix parameter for linear gradient (flutter#18208) 28d9985 Reland again "Remove layer integral offset snapping flutter#17112" (flutter#18160) 5e361f5 Roll src/third_party/skia c66cd987f7c0..0dc207f836da (5 commits) (flutter#18212) 716dbf0 Refactor GLFW embedding to support headless mode (flutter#18205) 23cca32 Manual Roll of Dart 39e0e75fcf...ce62ad2e8b (flutter#18211) 15f72b8 Support EventChannel C++ plugin API for Linux/Windows (flutter#17015) e5a7ca5 Handle leak of message handle when no engine present (flutter#18157) d3bde19 add docs to platformviewios (and some drive-by changes) (flutter#17593) b582d77 Roll src/third_party/skia 0066adefa97d..c66cd987f7c0 (4 commits) (flutter#18206) 5e7d6d0 Roll src/third_party/skia edea19858ccc..0066adefa97d (3 commits) (flutter#18203) 88b9d42 Remove the global engine entry timestamp (flutter#18182) 0dffdd5 Roll src/third_party/dart e86e4d61834a..ce62ad2e8b40 (13 commits) (flutter#18201) 56fe6d2 Roll src/third_party/skia 2871ab0727bf..edea19858ccc (3 commits) (flutter#18198) bcdf991 Fixed ChildSceneLayer elevation issue on Fuchsia. (flutter#18144) ede658e [profiling] CPU Profiling support for iOS (flutter#18087) d043923 Roll src/third_party/skia e3d1de7c5281..2871ab0727bf (1 commits) (flutter#18197) 807f191 Roll src/third_party/dart 733153eb517c..e86e4d61834a (6 commits) (flutter#18195) fe3f8df Roll src/third_party/skia 3b2db26c59d6..e3d1de7c5281 (4 commits) (flutter#18192) 34b0629 Roll fuchsia/sdk/core/mac-amd64 from jMJqf... to 1MVsE... (flutter#18194) e9b2afe Roll fuchsia/sdk/core/linux-amd64 from RpHTv... to MhpFP... (flutter#18191) 35fa006 Roll src/third_party/skia 88d04cb51acf..3b2db26c59d6 (1 commits) (flutter#18190) e54bb9e Roll src/third_party/dart 4da5b40fb6dc..733153eb517c (23 commits) (flutter#18188)

7e205b3 Roll src/fuchsia/sdk/mac from 1MVsE... to 4MCVP... (#18217) 9193d8f Roll src/third_party/skia 0dc207f836da..a14084ba1b41 (3 commits) (#18219) 403931f Add FlValue (#18185) 46a6d96 [SkParagraph] Copy text height behavior to the Skia paragraph style (#18178) e7ee47d [web] Implement matrix parameter for linear gradient (#18208) 28d9985 Reland again "Remove layer integral offset snapping #17112" (#18160) 5e361f5 Roll src/third_party/skia c66cd987f7c0..0dc207f836da (5 commits) (#18212) 716dbf0 Refactor GLFW embedding to support headless mode (#18205) 23cca32 Manual Roll of Dart 39e0e75fcf...ce62ad2e8b (#18211) 15f72b8 Support EventChannel C++ plugin API for Linux/Windows (#17015) e5a7ca5 Handle leak of message handle when no engine present (#18157) d3bde19 add docs to platformviewios (and some drive-by changes) (#17593) b582d77 Roll src/third_party/skia 0066adefa97d..c66cd987f7c0 (4 commits) (#18206) 5e7d6d0 Roll src/third_party/skia edea19858ccc..0066adefa97d (3 commits) (#18203) 88b9d42 Remove the global engine entry timestamp (#18182) 0dffdd5 Roll src/third_party/dart e86e4d61834a..ce62ad2e8b40 (13 commits) (#18201) 56fe6d2 Roll src/third_party/skia 2871ab0727bf..edea19858ccc (3 commits) (#18198) bcdf991 Fixed ChildSceneLayer elevation issue on Fuchsia. (#18144) ede658e [profiling] CPU Profiling support for iOS (#18087) d043923 Roll src/third_party/skia e3d1de7c5281..2871ab0727bf (1 commits) (#18197) 807f191 Roll src/third_party/dart 733153eb517c..e86e4d61834a (6 commits) (#18195) fe3f8df Roll src/third_party/skia 3b2db26c59d6..e3d1de7c5281 (4 commits) (#18192) 34b0629 Roll fuchsia/sdk/core/mac-amd64 from jMJqf... to 1MVsE... (#18194) e9b2afe Roll fuchsia/sdk/core/linux-amd64 from RpHTv... to MhpFP... (#18191) 35fa006 Roll src/third_party/skia 88d04cb51acf..3b2db26c59d6 (1 commits) (#18190) e54bb9e Roll src/third_party/dart 4da5b40fb6dc..733153eb517c (23 commits) (#18188)

googlebot added the cla: yes label Jun 5, 2018

edman changed the title ~~Surface border parameter in CupertinoSliverNavigationBar (#18152)~~ Surface border parameter in CupertinoSliverNavigationBar Jun 5, 2018

edman added 2 commits June 6, 2018 01:23

Surface border parameter in CupertinoSliverNavigationBar (flutter#18152)

5fa68f8

Add tests for CupertinoSliverNavigationBar border

8ecb483

edman force-pushed the border branch from 11ace4a to 8ecb483 Compare June 5, 2018 23:24

xster reviewed Jun 5, 2018

View reviewed changes

Remove unused keys, find by descendant

a6ac649

edman commented Jun 6, 2018

View reviewed changes

xster approved these changes Jun 6, 2018

View reviewed changes

xster merged commit 4bbd4ce into flutter:master Jun 6, 2018

edman deleted the border branch February 15, 2019 18:20

a-siva mentioned this pull request May 8, 2020

Manual roll of engine 7e205b37e5...3953c3ccd1 #56631

Closed

a-siva mentioned this pull request May 8, 2020

Manual roll of engine 7e205b37e5...3953c3ccd1 #56677

Merged

github-actions Bot locked as resolved and limited conversation to collaborators Aug 7, 2021

Conversation

edman commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xster commented Jun 5, 2018

Uh oh!

edman commented Jun 5, 2018

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!

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!

edman 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!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

edman commented Jun 5, 2018 •

edited

Loading