It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

Current implementation still contains

import '../material/color_scheme.dart';
import '../material/theme.dart';

as import in page_transition_theme.dart.

Current implementation still contains

import '../material/color_scheme.dart';
import '../material/theme.dart';

as import in page_transition_theme.dart.

widgets/ can't import material/, so this can't be a straightforward move to widgets/. I think the transition builders aren't tied to material/, but the page transitions theme (and any other theming stuff) probably is, so should remain.

widgets/ can't import material/, so this can't be a straightforward move to widgets/. I think the transition builders aren't tied to material/, but the page transitions theme (and any other theming stuff) probably is, so should remain.

Actually @victorsanni, Its both PageTransitionBuilder and PageTransitionTheme that depends on MaterialTheme right now.

I just wanted to chime in to say that I'm exploring adding a widgets/system_ui directory for this kind of stuff. Depending on if or what you are able to move out of Material, that might be the long term destination for it.

Sure @justinmc, I can keep this MR on hold until system_ui is created.

Its both PageTransitionBuilder and PageTransitionTheme that depends on MaterialTheme right now.

It's a hard requirement that the Widgets library can't import Material or Cupertino, or else it creates a circular dependency. I do not think there is a way to move PageTransitionTheme down without causing issues right now, so that should stay in Material. Ongoing investigations are happening around theming options in the Widget layer and that may provide a way to do that later.

So what should move down is PageTransitionsBuilder and the classes that extend from it. That still leaves the dependency on Theme and ColorScheme. Both are used to get a default background color for the transitions that need it. That gets partially resolved by adding a backgroundColor property to two of the builders. I suggest moving the backgroundColor property to PageTransitionBuilder so it can be used by all extended classes, and removing the theme look up from the builders. Then PageTransitionTheme, which is still in Material can see if backgroundColor is null and if so, it can set the backgroundColor using the Material theme.

Also the Cupertino transition builder will need the relevant code from Cupertino to move down as well.

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@MitchellGoodwin I have updated PR based on your suggestion, but when backgroundColor is not given in PageTransitionBuilder, I had to fallback to transparent.
Instances where transparent was used :-

1. FadeForwardsPageTransitionsBuilder.delegatedTransition
2. FadeForwardsPageTransitionsBuilder.buildTransitions
3. ZoomPageTransitionsBuilder.delegatedTransition
4. ZoomPageTransitionsBuilder.buildTransitions

Hi @rkishan516, can you draft this PR and open separate PRs migrating these builders individually? Will make it much easier to review.

Sure @victorsanni, Will do.

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@victorsanni, 1st PR for migrate through breaking this PR.
#174321