Slide Settings sub-sections into view like Appearance - #921
Open
ericgriffin wants to merge 1 commit into
Open
Conversation
Settings sections split into two navigation styles. Sections with a
dedicated page (Appearance, Diver Profile) push a child GoRoute, which
go_router wraps in a platform-adaptive MaterialPage, so they slide in.
The rest pushed '/settings?selected=<id>', which re-matched the
'/settings' route itself -- a bottom-nav tab root whose pageBuilder
returns a NoTransitionPage. Correct for tab switches, but it gave those
pushed sections a zero-length transition, so they snapped into place.
Give them a real child route, '/settings/section/:sectionId', with a
plain builder so go_router picks the same adaptive page Appearance gets.
Deliberately not fixed by returning a MaterialPage from the '/settings'
pageBuilder when '?selected=' is present: go_router mints a unique
pageKey for push() but reuses a stable one for go(), and the desktop
master-detail pane navigates with go(). Swapping the page's runtimeType
under an unchanged key fails Page.canUpdate and would animate the whole
split view on every left-pane click.
Legacy '?selected=' deep links still render. As a side benefit, deep
links to a section now materialize the parent chain, so the app-bar back
button pops instead of falling back to go('/settings').
Contributor
There was a problem hiding this comment.
Pull request overview
This PR standardizes Settings sub-section navigation so all sub-pages slide into view consistently (matching Appearance), by introducing a dedicated child route for section details instead of re-matching /settings via ?selected= (which inherits NoTransitionPage).
Changes:
- Added a
/settings/section/:sectionIdchild route (builder-based) to ensure platform-adaptive animated transitions. - Updated Settings mobile navigation to push the new child route instead of
/settings?selected=<id>. - Strengthened router/widget tests to assert non-zero transition durations and preserve
NoTransitionPageat the tab root.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/core/router/app_router.dart | Adds settingsSection child route under /settings using builder: to restore slide transitions. |
| lib/features/settings/presentation/pages/settings_page.dart | Promotes section detail page to a public widget and routes mobile section taps to the new child route. |
| test/core/router/app_router_test.dart | Verifies the new child route exists, uses builder, and /settings remains NoTransitionPage. |
| test/features/settings/presentation/pages/settings_page_test.dart | Updates navigation test harness to mirror real routing and asserts pushed route transition is non-zero. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
393
to
+405
| context.push('/settings/appearance'); | ||
| break; | ||
| default: | ||
| // For sections that don't have dedicated pages, show them in a | ||
| // detail page using query params. PUSH (not go): go() replaces the | ||
| // location in place, leaving nothing on the stack for the system | ||
| // back gesture to pop, so Android closed the whole app (#647). | ||
| final state = GoRouterState.of(context); | ||
| final currentPath = state.uri.path; | ||
| context.push('$currentPath?selected=$sectionId'); | ||
| // Sections without a dedicated page get the shared section route. | ||
| // PUSH (not go): go() replaces the location in place, leaving nothing | ||
| // on the stack for the system back gesture to pop, so Android closed | ||
| // the whole app (#647). | ||
| // | ||
| // This pushes a child route rather than '/settings?selected=<id>'. | ||
| // The latter re-matched the '/settings' tab root, whose pageBuilder | ||
| // returns a NoTransitionPage so bottom-nav tab switches do not | ||
| // animate -- which also robbed every pushed section of its slide-in. | ||
| context.push('/settings/section/$sectionId'); |
Contributor
|
📦 Build artifacts for this PR · commit
Artifacts expire in 7 days. Downloading requires being signed in to GitHub. macOS needs two extractions: unzip the downloaded artifact, then unzip the Updated automatically on each push. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In Settings, some sub-menus slid into view (Appearance) while others replaced the view instantly (About). All of them should slide.
Root cause
The two groups were not styled differently — they were structurally different go_router pages.
/settings/appearance— a childGoRoutewithbuilder:MaterialPage/settings?selected=<id>— re-matches/settingsitselfNoTransitionPageNoTransitionPageon/settingsis correct for a bottom-nav tab root: switching tabs must not animate. But the samepageBuilderwas reused whenever the route was imperatively pushed as a sub-page, so every query-param section inherited "no transition."Confirmed empirically rather than by inspection — the pushed route's
transitionDurationwas literally0:00:00.000000.Fix
Sections without a dedicated page now push a real child route,
/settings/section/:sectionId, with a plainbuilder:so go_router wraps it in the same adaptiveMaterialPageAppearance already gets.lib/core/router/app_router.dart— newsettingsSectionroutelib/features/settings/presentation/pages/settings_page.dart—_SettingsSectionDetailPagepromoted to publicSettingsSectionDetailPage(dropping a deadreffield);_MobileSettingsTile._navigateToSectionpushes the child routeLegacy
?selected=deep links still render, and the desktop master-detail split view is untouched.Why not just swap the page type on
/settingsReturning a
MaterialPagefrom the/settingspageBuilderwhen?selected=is present looks like the smaller fix, but it would break desktop. go_router mints a uniquepageKeyforpush()and reuses a stable one forgo()— and the desktop master-detail pane navigates withgo().Page.canUpdaterequires a matchingruntimeType, so flipping the page type under an unchanged key would slide the entire split view on every left-pane click. The child route removes the ambiguity instead of working around it.Side benefit
/settings?selected=aboutproduced a single-page stack, socanPop()was false and the app-bar back button fell back togo('/settings')./settings/section/aboutmakes go_router materialize the parent chain, so back pops naturally.Testing
test/core/router/app_router_test.dart— the section child route exists and usesbuilder:(not a custompageBuilder), and/settingsitself still returns aNoTransitionPageso tab switches stay instanttest/features/settings/presentation/pages/settings_page_test.dart— tapping a section pushes/settings/section/dataand the resulting route has a non-zerotransitionDuration; the#647helper now mirrors the real route config (tab root + child route) instead of a stub builderThe new assertion was checked for discriminating power: an initial
hasRunningAnimationsversion passed against the buggy code, because theListTiletap ripple animates either way. It was replaced with an assertion on the pushed route'stransitionDuration, verified red before the fix and green after.Full suite: 15,714 passed, 15 skipped, 0 failures.
flutter analyzeclean,dart formatreports no changes.Note for reviewers
There is parallel in-flight work adding
lib/core/router/back_navigation.dartwith aresolveUpLocation()helper that unwinds a URL by dropping one path segment. It is not onmain, so there is no conflict today — but once it lands,/settings/section/aboutwould resolve "up" to/settings/section, which is not a valid route. That helper will need to drop both segments or special-case this route.