Skip to content

add web_dashboard sample #306

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

Closed
wants to merge 4 commits into from
Closed

add web_dashboard sample #306

wants to merge 4 commits into from

Conversation

johnpryan
Copy link
Contributor

This adds a responsive master-detail app with browser URL navigation. The app combines a new AdaptiveScaffold widget that switches between a Drawer, NavigationRail, and BottomAppBar for navigation. The body of the Scaffold is a Navigator that handles three routes using onGenerateRoute. The URL stays in sync with the routing in the app.

Right now, there are two Navigators, the root Navigator and the child Navigator. The root Navigator forwards the initial route to the child using onGenerateInitialRoutes and passing the RouteSettings. The AppShell widget's keeps the AdaptiveScaffold Navigator in sync.

Copy link
Contributor

@RedBrogdon RedBrogdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One real question and a bunch of nits. :)

@@ -0,0 +1,17 @@
# web_dashboard
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It'd be a good idea to add a couple sentences:

  • Explain that this is an in-progress sample.
  • Point out that it's just for the web, and shouldn't be built for Android/iOS/desktop.

You can also link from here to the README in /experimental, which will tell people about the need to use the master channel.


import 'src/adaptive_scaffold.dart';

/// The [Key] for the inner [Navigator]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Period at the end of the comments (here and below).

Widget build(BuildContext context) {
return MaterialApp(
// Forward the initial route to the app shell.
onGenerateInitialRoutes: (initialRoute){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: space between the closing paren and the opening brace.

Related: Nothing in /experimental gets touched by CI, so formatting and analysis aren't checked automatically. I have an alias I use a lot that does both, in case it's of use to you:

alias fa='find . | grep "\.dart$" | xargs flutter format && flutter analyze'

// initial build phase. Right now this is being called in the wrong
// order. (Refreshing the browser with the initial route set to
// '/settings' invokes onGenerateRoute with '/' instead of
// '/settings'.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Closing paren.

},
),
currentIndex: _currentIndex,
destinations: [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can this be a const list?

}

/// A widget that adapts to the current display size, displaying a [Drawer],
/// [NavigationRail], or [BottomAppBar]. Navigation destinations are defined in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: BottomNavigationBar rather than BottomAppBar?

return Scaffold(
appBar: AppBar(title: widget.title,),
body: Row(
children: <Widget>[
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitL: No type specifier necessary here.

var navState = _navigatorKey.currentState as NavigatorState;
navState.pushNamed(_routeForIndex(idx));
},
floatingActionButton: FloatingActionButton(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can imagine wanting this button to have different effects based on which page is loaded into the navigator inside the AdaptiveScaffold. Is that something that would be supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the AdaptiveScaffold would cover that case. The user would be responsible for applying any effects (or hiding the FAB altogether) based on the currentIndex.

@johnpryan johnpryan marked this pull request as ready for review February 18, 2020 21:04
@johnpryan johnpryan mentioned this pull request Feb 20, 2020
@johnpryan
Copy link
Contributor Author

closing in favor of #333, which moves this into an experiments/ directory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants