Fix sample code and update docs #11257
Conversation
goderbauer
force-pushed
the
sampleCodeFix
branch
from
b175fd8 to
a3aa0b4
Compare
| /// State<RouteAwareWidget> createState() => new RouteAwareWidgetState(); | ||
| /// } | ||
| /// ``` | ||
| class RouteObserver<T extends Route<dynamic>> extends NavigatorObserver { |
There was a problem hiding this comment.
I don't understand the "if (route is T && previousRoute is T)" part of the API here.
There was a problem hiding this comment.
For the Firebase Analytics use case we'd like to be informed about PageRoutes that are pushed onto other PageRoutes (or popped from them) to track page transitions (see https://github.com/flutter/plugins/blob/master/packages/firebase_analytics/lib/observer.dart#L51). This RouteObserver enables us to do just that.
During the initial PR we saw that a PageRouteObserver could be generalized to observe any type of route changes and what you see here is the result of that. If you want to be informed about route changes of any type, you use a RouteObserver<Route>. If you are only interested PageRoute changes, you use RouteObserver<PageRoute>.
There was a problem hiding this comment.
Having said that, if we don't want this in the framework, we can also just put it in the Firebase Analytics Plugin (and then duplicate it for the Google Analytics Plugin). However, my discussion with @collinjackson indicated that it makes sense to have this logic in the framework to share it across the plugins.
There was a problem hiding this comment.
Don't tell me, put it in the documentation. :-)
| /// ```dart | ||
| /// class RouteAwareWidgetState extends State<RouteAwareWidget> with RouteAware { | ||
| /// | ||
| /// final RouteObserver routeObserver = new RouteObserver(); |
There was a problem hiding this comment.
Why instantiate this class rather than put all its logic into RouteAware?
There was a problem hiding this comment.
As the documentation states, RouteObserver is a NavigatorObserver, so you'd most likely only instantiate one, register it with the Navigator and then register multiple Widgets (that want to be aware of changes to their current route) to the RouteObserver.
There was a problem hiding this comment.
We should make that clear in the sample code then. You should assume that the sample code is what will be copied-and-pasted. It should be the most pedantically correct thing, because it represents the best that we can hope to see in deployed code.
There was a problem hiding this comment.
Done, but I think that made the example worse. Previously, the example had a clear focus: How do I implement RouteAware correctly in a widget. Now, the example is convoluted with other stuff (e.g. how to register the RouteObserver with the Navigator, something that is not specific to the RouteObserver and common to all NavigatorObservers).
goderbauer
force-pushed
the
sampleCodeFix
branch
from
f7b9791 to
7daa621
Compare
goderbauer
force-pushed
the
sampleCodeFix
branch
from
7daa621 to
0cacff4
Compare
| /// on top of their own route of type `T` or popped from it. This is for example | ||
| /// useful to keep track of page transitions, e.i. a `RouteObserver<PageRoute>` | ||
| /// will inform subscribed [RouteAware]s whenever the user navigates away from | ||
| /// the current page route to another page route. |
There was a problem hiding this comment.
This seems like a weird feature. What if they're going from a PageRoute to a DialogRoute to a PageRoute to a PageRoute? Why should they only be notified once?
There was a problem hiding this comment.
For analytics, you mostly care about transition from one full-screen page to another (at least for this use case). You don't care about transient routes (e.g. an error dialog informing you that your phone number is required to have 10 digits should not show up as a page transition in analytics). Those transient route-specific sub-routes should be popped off the navigation stack before moving to the next page route (and from my experience that's how almost all apps behave).
That's how leafy wants Google Analytics to be wired up and that also made the most sense to me when wiring up Firebase Analytics.
If you really want to be informed about all route changes, you're free to instantiate RouteObserver<Route> as mentioned in the docs in the paragraph below.
| /// [RouteAware] in its [State] and subscribe it to the [RouteObserver]: | ||
| /// | ||
| /// ```dart | ||
| /// // Register the RouteObserver as a navigation observer. |
| /// | ||
| /// } | ||
| /// | ||
| /// class RouteAwareWidget extends StatefulWidget { |
There was a problem hiding this comment.
We should put this before the State
| /// ## Sample code | ||
| /// | ||
| /// To make a [StatefulWidget] aware of its current [Route] state, implement | ||
| /// [RouteAware] in its [State] and subscribe it to the [RouteObserver]: |
goderbauer
force-pushed
the
sampleCodeFix
branch
from
dbad588 to
db93402
Compare
|
|
|
Bonus points if you also document all the other members on RouteObserver. :-) |
Done. Where can I claim my bonus points? :) |
|
I believe you can redeem them at the gift shop. :-) new changes LGTM |
3 participants
No description provided.