add web_dashboard API #333
Conversation
redbrogdon
left a comment
redbrogdon
left a comment
There was a problem hiding this comment.
Mostly nits with a few actual questions. This looks really good. It's great to see tests already in place as you're landing the API stuff, too.
| @@ -0,0 +1,17 @@ | |||
| # web_dashboard | |||
There was a problem hiding this comment.
Consider adding a line in here specifically telling people not to run the app on mobile or desktop.
| return item..id = id; | ||
| } | ||
|
|
||
| Stream<List<Item>> subscribe() { |
There was a problem hiding this comment.
IIRC, in other projects I'm typically see something like:
Stream<List<Item>> get itemStream => _streamController.stream;
Is this a pattern you've seen elsewhere?
| /// BottomNavigationBar, or NavigationRail, as well as the correct page in the | ||
| /// body of the Scaffold. | ||
| class AppShell extends StatefulWidget { | ||
| final String routeName; |
There was a problem hiding this comment.
Is this used solely for the initial route when building the navigator later? If so, consider renaming it to initialRouteName or something similar.
There was a problem hiding this comment.
considering this fixed since I took this file out.
| @@ -0,0 +1,17 @@ | |||
| # web_dashboard | |||
| // order. (Refreshing the browser with the initial route set to | ||
| // '/settings' invokes onGenerateRoute with '/' instead of | ||
| // '/settings'.) | ||
| SchedulerBinding.instance.addPostFrameCallback((dur) { |
There was a problem hiding this comment.
We should set aside some time to talk about what you've needed to do here. I have no doubt that you've got the best available pattern, but having something this complicated in one of our samples is a DevRel smell, so to speak, and we should try to effect change to make the API better.
There was a problem hiding this comment.
Removed, since this is more of an experiment than a best-practice.
|
@redbrogdon ready for another look |
2 participants
This is the latest version of the web_dashboard sample. Replaces #306