Material Fade Transition Configuration; showModal implementation

[shihaohong]

Description

1. Implements FadeTransitionConfiguration, which is a modal configuration that generates a ModalRoute with the Material Fade Transition.
2. Implements showModal, which takes in a ModalConfiguration object and pushed a _ModalRoute onto the Navigator. This allows for easy customization of modal transitions in the future. This was used to implement the Material FadeTransitionConfiguration described in 1.

Fade transition

[goderbauer]

nit: this should probably be a relative import.

[goderbauer]

Why do we need the Column? Seems like it only has one child?

[shihaohong]

Without the Column widget, the ConstrainedBox somehow does not respect the maximum width constraints and takes up the entire width of the screen instead. My guess is this is because it's the root widget of a new page, but I'm not entirely sure. I could investigate this and see if I could substitute in a widget that might make more sense here.

[goderbauer]

That does seem odd. I would expect a Center(child: SizedBox(...)) to work as expected. Can you show me what it looks like without?

[shihaohong]

SizedBox worked, but ConstrainedBox didn't when I tried. I just swapped it out

[goderbauer]

This comment is kinda strange, nothing in this class seems to imply that it is a modal. It's only one because it was rendered with showModalWithFadeTransition, no?

[shihaohong]

Right, that makes sense. I'll rename the widget

[goderbauer]

nit: it seems like this paragraph should just be a continuation of the previous one?

[goderbauer]

I believe in the other showDialog type methods we deprecated the child in favor of a builder. We should probably just have a builder argument here?

[goderbauer]

Can we avoid looking up the string if the barrier is not dismissible?

[goderbauer]

Wondering if we should just keep this class private and only make showModalWithFadeTransition the entry point?

[shihaohong]

I think my original thought process here was that I felt that the Material library's _DialogRoute would have been nice to use here if it were public and more extensible.

Thinking about it now, I guess it wouldn't really matter since we're taking a similar approach as _DialogRoute with the _FadeModalRoute. I'll change this back to private.

[goderbauer]

From an API point of view: Would it be maybe better to have a showModal function that can be configured with different transitions, e.g. by passing in a transition object of some kind?

[shihaohong]

I'll look into it. I think it'd take some refactoring of the existing FadeModalRoute into something more generic and have some "route configurations" class that passes along all information used by the new general ModalRoute, such as transition duration, barrier label, etc.

[shihaohong]

I just refactored everything to have a generic showModal function that uses a configuration object for determining the particulars of the modal transitions.

This means that I'll also have to incorporate tests for the general showModal function as well
@goderbauer PTAL

[goderbauer]

I like this approach.

[goderbauer]

Just use a SizedBox here since you're giving tight constraints?

[goderbauer]

That does seem odd. I would expect a Center(child: SizedBox(...)) to work as expected. Can you show me what it looks like without?

[goderbauer]

nit: new line between package: and non-package imports.

[goderbauer]

nit: add blank line above?

[goderbauer]

Instead of making this a getter that returns a function: Just have this be the function? You can just tear it of by passing "FadeTransitionConfiguration.transitionBuilder" around?

[goderbauer]

PopupRoute?

[goderbauer]

Suggested change

	if (configuration.barrierDismissible ==true&& configuration.barrierLabel ==null) {
	if (configuration.barrierDismissible && configuration.barrierLabel ==null) {

[goderbauer]

Mention this parameter in the docs above?

[goderbauer]

assert that barrierDismissible is not null?

[goderbauer]

missing license header

[goderbauer]

Looks like this could be private?

[goderbauer]

This will probably need a few more tests?

[shihaohong]

Yup! It's still a draft PR so it's not quite there yet -- I'll also need to write tests for modal.dart and address the earlier code review comments

[goderbauer]

LGTM

[goderbauer]

nit: document that barrierDismissible cannot be null

[goderbauer]

I believe you can simplify all of these overridden getters in the following way:

In the constructor just do:

_ModalRoute({
this.barrierColor,
// ...
});

... and instead of having the getter and the private field here just do:

@overridefinalColor barrierColor;

[goderbauer]

Can barrierDismissible, transitionDuration and reverseTransitionDuration legally be null? If not, assert and document?

[shihaohong]

barrierDismissible legally cannot be null, I introduced an assert for it. Since transitionDuration and reverseTransitionDuration are typically set by overrides to the properties, those can remain as-is

[goderbauer]

Move these two properties above the method to have all properties grouped together?

[goderbauer]

We should decide wether we want these to be supplied via overrides or by passing them in in the constructor. If we take them via constructor, then we should assert that they have the right value here (and have subclasses pass them in in the super constructor call). If we want them to be provided via override, we should remove them from the constructor and just have abstract Duration get transitionDuration getters in the class.

I am voting to do the first option.

[goderbauer]

Does it make sense to make these configurable? I would just pass them directly in the super constructor below:

super(
barrierColor:Colors.black54,
transitionDuration:constDuration(milliseconds:150),
// etc.
)

[shihaohong]

I was thinking maybe users might want to modify this transition for their own use. However, if we prefer that users are forced to use the values we provide them, we can just do this instead. Let me know what you think.

[goderbauer]

We can also make them configurable. That's fine too.

[goderbauer]

same here.

[shihaohong]

We can make these non-configurable, since it's just a test configuration

[goderbauer]

LGTM

[goderbauer]

We can also make them configurable. That's fine too.