Documentation Improvement

[brianegan]

Woohoo! Great job :) This was much easier to use to get started -- a great improvement overall.

I've just gone through my experiment again using the new docs and took these notes, please let me know if I can provide any more info!

Navigator Documentation Walkthrough

Getting started

• Does not include instructions for adding navigator and build_runner to pub spec, will result in error: “Could not find package "build_runner". Did you forget to add a dependency?”
• “The generated file will be lib/src/router.g.dart and will have several helper methods”
  • This would be a good place to describe what is being generated!
• Maybe add just a Scaffold + AppBar to the tutorial screen so ya don’t get the crazy red text :)
• Should contain have a section on adding a second route
  • Create an “About” screen
  • Add the route to the router
  • Re-generate the router, discuss watch
  • Use the generated Nuvigator for navigation
• Important: Add a Note to use builder instead of home! This actually tripped me up — Since Nuvigator implements the call method, it kinda looks like the builder is taking in any normal child rather than a TransitionBuilder. This was the cause of a bug on my end -- I was accidentally using home b/c I missed that detail!

Route Argument/Parameters

• By providing named routes to an argument, it generates a conflicting class called “TutorialScreen,” which we already defined in the "Getting Started" section
• Generated Screen classes still feel a bit weird. 1) The naming can conflict with your own Widgets, 2) Extending a class to get data from the tree just doesn’t feel very “Fluttery” , 3) provides 2 ways to do the same thing, 4) Kinda feels "wrong" to pass a parent context into a Widget
• Adding the arguments to the tutorial route results in the following error:
  • I/flutter ( 6079): The following NoSuchMethodError was thrown building Builder:
  • I/flutter ( 6079): The method '[]' was called on null.
  • I/flutter ( 6079): Receiver: null
  • I/flutter ( 6079): Tried calling:
  • Overall, this should provide a much cleaner error message, such as “The X route requires arguments, but none were provided. Please use the generated navigation methods to perform navigation, or provide initialArguments to Nuvigator.”
• Fixed by adding initialArguments
  • Found out this actually took a Map instead of dynamic
  • The TutorialArgs class does not provide a toMap function, meaning I had to program the initialArgs by hand by looking at the parse method.
• Is the TutorialArgs class even necessary? Seeing that you can also pass the data directly into the Widget means there are 3 ways of getting the arguments from the route?

More code questions

• Should *Navigation be *Navigator? E.g. TutorialNavigator instead of TutorialNavigation? Then it reads like TutorialNavigator.of(context).toSomeRoute, which is a bit more like Navigator.of(context).
• Could this package generate extension methods or a mixin on TutorialRouter to reduce the number of necessary classes? (after a brief experiment, this might not work that well)

[leoiacovini]

Hey @brianegan, tks again for the feedback, we are reflecting upon your feedback and planning to make some changes to make things more explicit and idiomatic :). As soon as we have something I will let you know, just wanted to write an answer here before.

Just to get some inputs: We thought about doing something with extensions but nothing too different came to our minds... (because we would need to add static methods, and I guess extensions do not support it right now). Do you have something different in mind? The furthest I could think of was:

• Provide the router instance via context to the screens
• Extends a navigator method on the router to retrieve the RouterNavigator instance
• You would be able to do something like: TutorialRouter.of(context).navigator

I have not tested it, so I may have missed something haha.

[brianegan]

Nah I think you're exactly right when it comes to mixins / extension methods -- after making that note I actually tried a quick experiment using both, but ran into the same problem: ya need static methods!

Overall, my goal was to see if I could reduce the number of generated classes to simplify things.

[leoiacovini]

Hey @brianegan, I did some changes to the overall library API based on your feedback, my goal was mainly to simplify the overall complexity of classes/names/abstraction and reduce it to the essentials, also trying to shape up the expected usage without providing too many options to do the same thing (like you pointed out). I tried to make things more explicit and using the same set of names wherever possible. If you can take a look at the changelog changes here:

https://github.com/nubank/nuvigator/pull/20/files#diff-4ac32a78649ca5bdd8e0ba38b7006a1e I think they make a good summary of the changes.

The example was tided up a bit to make easier to understand what is going on. We are going to update this documentation PR after it. The recommended usage is to rely in the arguments provided by the Router method, or if you wish to get it down in you Widget tree, you still can call the Args*.of(context), other options were removed. We think this makes things more clear and explicit overall and works out as a good separation of concerns. We also merged the Navigation* class with the Router using extension methods, and now, things should be pretty much easier to remember and use.

Let me know if I got your point correctly with those changes =D

[brianegan]

@leoiacovini Oh my, so sorry -- somehow missed that last request / github notification. Just left some comments on the other PR.