Feat: Add opaque, isActive, isFirst, popDisposition aspects for ModalRoute #167324
Conversation
github-actions
bot
added
framework
| @@ -2509,6 +2509,35 @@ void main() { | |||
| moreOrLessEquals(xLocationIntervalTwelve, epsilon: 0.1), | |||
| ); | |||
| }); | |||
|
|
|||
| testWidgets('ModalRoute.isFirstOf only rebuilds when first route state changes', ( | |||
There was a problem hiding this comment.
Would you mind adding a similar test for each of the other of methods that you added as well?
There was a problem hiding this comment.
Sure, I will add.
rkishan516
force-pushed
the
modal-route-aspects
branch
from
288d8ac to
721d47a
Compare
| @@ -1321,6 +1341,24 @@ abstract class ModalRoute<T> extends TransitionRoute<T> with LocalHistoryRoute<T | |||
| static RouteSettings? settingsOf(BuildContext context) => | |||
| _of(context, _ModalRouteAspect.settings)?.settings; | |||
|
|
|||
| /// Returns [ModalRoute.isActive] for the modal route most closely associated | |||
| /// with the given context. | |||
There was a problem hiding this comment.
I see that settingsOf and others above include a bit more in their docs, should you add them for the new parameters too?
/// Returns null if the given context is not associated with a modal route.
///
/// Use of this method will cause the given [context] to rebuild any time that
/// the [ModalRoute.settings] property of the ancestor [_ModalScopeStatus] changes.
There was a problem hiding this comment.
Sure, thanks for the suggestion.
There was a problem hiding this comment.
We should not mention _ModalScopeStatus in the doc since it's a private widget.
I also consulted with AI, who said the original version might be slightly ambiguous (since the reader might the the BuildContext itself would be rebuilt) and suggested the following. What do you think?
/// Calling this method creates a dependency on the [ModalRoute] associated
/// with the given [context]. As a result, the widget corresponding to [context]
/// will be rebuilt whenever the route's [ModalRoute.settings] changes.There was a problem hiding this comment.
I agree with not mentioning private members and I like the rewrite 👍
I don't know. Because if we switch to selector pattern, than these will become just helper methods and soon we need to deprecate. I will rather wait for #165289, that which direction it will go and based on that take a call on this. Wdyt ? |
rkishan516
force-pushed
the
modal-route-aspects
branch
from
721d47a to
a1e40d0
Compare
|
@rkishan516 This should also fix #162009 right? You can add that to the OP then. |
Done, have updated. |
Ok I agree, let's wait for #165289. So this PR can stay open for now. I'll prioritize reviewing that one. |
| @@ -1321,6 +1341,24 @@ abstract class ModalRoute<T> extends TransitionRoute<T> with LocalHistoryRoute<T | |||
| static RouteSettings? settingsOf(BuildContext context) => | |||
| _of(context, _ModalRouteAspect.settings)?.settings; | |||
|
|
|||
| /// Returns [ModalRoute.isActive] for the modal route most closely associated | |||
| /// with the given context. | |||
There was a problem hiding this comment.
We should not mention _ModalScopeStatus in the doc since it's a private widget.
I also consulted with AI, who said the original version might be slightly ambiguous (since the reader might the the BuildContext itself would be rebuilt) and suggested the following. What do you think?
/// Calling this method creates a dependency on the [ModalRoute] associated
/// with the given [context]. As a result, the widget corresponding to [context]
/// will be rebuilt whenever the route's [ModalRoute.settings] changes.
rkishan516
force-pushed
the
modal-route-aspects
branch
from
a1e40d0 to
27bfdc5
Compare
I have updated the docs based on your suggestion. |
justinmc
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
…Route (flutter#167324) Feat: Add opaque, isActive, isFirst, popDisposition aspects for ModalRoute fixes: flutter#167058 fixes: flutter#162009 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
Feat: Add opaque, isActive, isFirst, popDisposition aspects for ModalRoute
fixes: #167058
fixes: #162009
Pre-launch Checklist
///).