Repository navigation
Conversation
The home screen's accounts pill shows a down caret, but the screen slid in from the right. Open it as a fullscreen dialog route so it rises from the bottom on both platforms, and dismiss it with a down caret.
n13
left a comment
n13
left a comment
There was a problem hiding this comment.
Verdict: Approve
Reviewer: GPT-6.1 Sol
Reviewed the complete diff at ec931ac9cb4dca76d3f28f25bf8b37659675fe75 against base 29277c8ed5bb62b4853c800e082ae168fa8e7dd6. No blocking findings.
All Accounts openings use the shared navigation helper, so the fullscreen Cupertino presentation applies consistently. The route name and highlight parameter remain intact for add/import return flows. The dismiss caret retains the existing pop behavior, and child screens keep their existing routes. The shared back-button change is additive and preserves the default chevron for other callers.
Validation:
git diff --checkpassed.- Read-only workspace formatting check (
dart run melos exec -- dart format --output=none --set-exit-if-changed lib test --line-length=120) passed with no changes; it emitted missing-package warnings because local bootstrap was incomplete. - Verified CI run 40270261105 succeeded at this exact head, including formatting, analysis, mobile tests, SDK tests excluding native-tagged tests, and cold-wallet tests.
- Local tests and analysis could not run: installed Flutter 3.44.4 differs from the pinned 3.47.7, and
dart run melos bootstrap --offlinerejected the committed lockfile rather than changing six SDK-pinned dependencies. Physical-device animation and gestures were not exercised locally.
|
|
||
| return ScaffoldBase( | ||
| appBar: V2AppBar(title: l10n.accountsSheetTitle), | ||
| appBar: V2AppBar( |
There was a problem hiding this comment.
Isn't this kind of weird to show arrow down for back button? shouldn't we just show close icon instead?
There was a problem hiding this comment.
Agreed, switched to an X in c113c86. It's the same ghost close button (QuantusIconButton.ghost(icon: Icons.close)) the swap and mining reward sheets use, on the right of the title bar, and the left back button is hidden. That also dropped the AppBackButton icon parameter, so back_button.dart is unchanged now.
A down caret reads oddly as a back button. Use the ghost close button the swap and mining reward sheets already show, and drop the back button's icon parameter that only the caret needed.
n13
left a comment
n13
left a comment
There was a problem hiding this comment.
Not reviewing again — no new commit or comment since the last review.
The accounts pill on the home screen shows a down caret, but tapping it slid the Accounts screen in from the right.
openAccountsScreennow pushes aCupertinoPageRoute(fullscreenDialog: true), so the screen rises from the bottom on iOS and Android.MaterialPageRoutewithfullscreenDialogonly does that on iOS.