Skip to content
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.

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

Added a Backdrop demo to the Gallery #15579

Merged
merged 8 commits into from Mar 16, 2018

Conversation

Projects
None yet
4 participants
@HansMuller
Copy link
Contributor

commented Mar 15, 2018

A alternative way to present one page from a set of pages.

The initially selected page appears stacked on top of the "backdrop". Only the backdrop's title and menu icon are visible.

backdrop_home

Tapping the selected page's header or the backdrop's menu icon exposes the backdrop and slides the selected page to the bottom of the screen.

backdrop_menu

HansMuller added some commits Mar 15, 2018

@googlebot googlebot added the cla: yes label Mar 15, 2018

HansMuller added some commits Mar 15, 2018

@gspencergoog
Copy link
Contributor

left a comment

This looks good, in general. Since this is "demo" code, though it might be nice to have some more comments to describe what's going on.

return renderBox.size.height;
}

// By design: the panel can only be opened with a swipe. To close the panel

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Mar 15, 2018

Contributor

It can't be closed by swipe?

This comment has been minimized.

Copy link
@HansMuller

HansMuller Mar 15, 2018

Author Contributor

Our UX designer was really clear about that, yes.

@@ -12,7 +12,7 @@ class GalleryTheme {
}

const MaterialColor _kPurpleSwatch = const MaterialColor(
500,
0xFF6200EE,

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Mar 15, 2018

Contributor

Whoa. I guess any int will do. :-)

This comment has been minimized.

Copy link
@HansMuller

HansMuller Mar 15, 2018

Author Contributor

I should fix that with a constant. It's just the value of the 500 entry.

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Mar 15, 2018

Contributor

Yeah, a constant seems like a good idea.

@@ -81,6 +81,13 @@ List<GalleryItem> _buildGalleryItems() {
buildRoute: (BuildContext context) => const VideoDemo(),
),
// Material Components
new GalleryItem(
title: 'Backdrop',

This comment has been minimized.

Copy link
@xster

xster Mar 15, 2018

Contributor

Should this be called 'Navigation' or some such? I thought at first it was a demo about blur filters or something.

This comment has been minimized.

Copy link
@HansMuller

HansMuller Mar 15, 2018

Author Contributor

Backdrop is what the people who came up with it call it.

@@ -128,6 +128,9 @@ Future<Null> runSmokeTest(WidgetTester tester) async {
final Finder finder = findGalleryItemByRouteName(tester, routeName);
Scrollable.ensureVisible(tester.element(finder), alignment: 0.5);
await tester.pumpAndSettle();
// The backdrop demo has no back button so it can't be smoked.
if (routeName == '/material/backdrop')

This comment has been minimized.

Copy link
@xster

xster Mar 15, 2018

Contributor

How do you exit this demo on iOS?

I hit the same problem on #15324 and after chatting with @yjbanov, we'll add a tester.backPage() or something. But it might not help here.

This comment has been minimized.

Copy link
@HansMuller

HansMuller Mar 15, 2018

Author Contributor

A user could exit on iOS with an edge swipe.

If we had tester.backPage() to use as a fallback when no back button was found, then this demo could be smoked.

This comment has been minimized.

Copy link
@xster

xster Mar 15, 2018

Contributor

Seems a bit strange that there'd be a hamburger menu or a close button on the corner and a swipe triggers neither and closes everything. Should there be an explicit exit demo button somewhere instead? (which then can be Tooltipped and smoke tested)

This comment has been minimized.

Copy link
@HansMuller

HansMuller Mar 15, 2018

Author Contributor

I added a back button to the backdrop. It seems a little out of place, but it does give the iOS user a relatively obvious escape route. And it smokes.

HansMuller added some commits Mar 15, 2018

@gspencergoog

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2018

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

HansMuller added some commits Mar 15, 2018

@xster

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2018

LGTM

@HansMuller HansMuller merged commit c599662 into flutter:master Mar 16, 2018

4 checks passed

cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
flutter-build

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.