UI: refactor universal back button and deduplicate navigation header - #137
Conversation
theanmolsharma
left a comment
There was a problem hiding this comment.
Unifies the back chevron across every navigationStyle screen by replacing the !headerLeft && !isFirstRouteInStack gate with a canGoBack check inside headerLeft, deletes withBackChevron from SendDetailsStack, adds a backButtonIcon token, drops the redundant headerBackButtonDisplayMode configs, and tidies ImportWallet's headerLeft.
This is the follow-up I asked for on #131 and the approach is right. tsc --noEmit and eslint are both clean on the branch as it stands. Three things before merge — two are inline, and the blocking one is below because it spans files.
Blocking — conflicts, and one auto-merges into code that won't compile
#131 landed as 8d650afbd, so this branch is now CONFLICTING. git merge-tree origin/master <branch> flags two files — but the dangerous one isn't among them.
navigation/SendDetailsStack.tsx auto-merges silently and produces a broken file. #131 added withBackChevron to SelectFee and Confirm; this PR deletes the helper definition plus the single call site it knew about. Git merges both sides cleanly, and the result contains:
line 38 options={navigationStyle({ title: loc.send.network_fee_header }, withBackChevron)(theme)}
line 43 options={navigationStyle({ title: loc.send.confirm_header }, withBackChevron)(theme)}
…with zero definitions of withBackChevron left in the file, and its imports removed. tsc catches this as TS2304, but git reports no conflict — so resolving only the two flagged files and trusting the rest ships a broken build. After rebasing, remove three call sites (lines 54, 61, 66 on current master), not one.
navigation/DetailViewScreensStack.tsx — CONFLICT. This PR removes headerBackButtonDisplayMode: 'default' from the TransactionStatus screen block, which #131 deleted in its entirety. Drop the hunk; it's moot.
screen/wallets/ImportWallet.tsx — CONFLICT. #131 hoisted a module-level ImportWalletHeaderLeft; this PR uses a PascalCase useCallback inside the component. Two fixes for the same lint warning. Master's version is already in place and doesn't recreate the component identity per render, so taking master's side and dropping this hunk is the smaller change.
components/themes.ts auto-merges cleanly.
Checked and clear
headerBackButtonDisplayModehad exactly one occurrence on master, so removing it plus the'default'override is complete — and it's moot anyway once a customheaderLeftalways replaces the native button.- Root screens are unaffected:
WalletsListhascanGoBack === falseand correctly rendersnull.
Suggested order
Rebase onto 8d650afbd, resolve the two flagged conflicts, then manually verify SendDetailsStack.tsx even though git says it's fine.
| }} | ||
| /> | ||
| ); | ||
| if (!headerLeft) { |
There was a problem hiding this comment.
Non-blocking, but worth a comment in the code at minimum.
Broadening the gate from !headerLeft && !isFirstRouteInStack to !headerLeft means the CloseButtonPosition.Right case now also gets a left chevron, since that branch sets only headerRight and leaves headerLeft undefined. That would give modals two dismiss affordances — chevron on the left, X on the right.
It's harmless today only because Right is unreachable: getCloseButtonPosition returns it just when isFirstRouteInStack && isModal, and isModal reads route.params?.presentation while every call site passes presentation as a navigationStyle option, never a route param. So isModal and isFormSheet are permanently false, Right never fires, and styles.buttonFormSheet is dead code too. There are no explicit Right call sites either — only Left, at AddWalletStack.tsx:39 and DetailViewScreensStack.tsx:154.
Since this is the PR refactoring this function, either fix the params/options mismatch or leave a note — otherwise whoever repairs isModal later inherits double dismiss buttons with no idea why.
There was a problem hiding this comment.
Fixing isModal to read opts.presentation is the better call — that's the root cause, not just the comment I asked for. Thanks.
But it landed alongside reverting the gate to !headerLeft && !isFirstRouteInStack while withBackChevron stayed deleted, and those two together remove the chevron from the one screen that needed it.
For SendDetails:
- it's
initialRouteNameofSendDetailsStack, soisFirstRouteInStack === true - its opts carry no
closeButtonPositionand nopresentation, socloseButton === Noneand neither close branch fires !headerLeft && !isFirstRouteInStack→true && false→ false, soheaderLeftstays undefined- and
withBackChevron, which used to cover exactly this case, is gone
So SendDetails falls through to whatever native-stack renders for an unset headerLeft. The result is non-uniform — pushed screens get the custom chevron, stack roots don't — which is the opposite of what the PR title promises.
There's a second effect worth knowing about: removing headerBackButtonDisplayMode: 'minimal' is now a live change rather than a no-op. In the previous revision headerLeft was always set, so no native back button could appear and dropping the option was harmless. Now that first-route screens fall through to native, those buttons use the default display mode — on iOS that shows the previous screen's title as a back label. Worth checking SendDetails on a device.
Suggested gate
The cleanest condition is the close-button state itself:
if (closeButton === CloseButtonPosition.None) {
headerLeft = (props: any) => (props.canGoBack ? <HeaderBackButton … /> : null);
}That states the actual intent — supply a chevron only when there's no close button — and it resolves everything at once:
SendDetailsgets its chevron back- a future first-route modal gets only its right-hand X, so the double-dismiss problem I originally raised can't happen (your
isModalfix is what makes this reachable and therefore worth guarding) - true roots like
WalletsListstay clean viacanGoBack headerBackButtonDisplayMode: 'minimal'becomes a genuine no-op again, so its removal is safeprops.canGoBackbecomes load-bearing again — with!isFirstRouteInStackstill in the gate it's very nearly unreachable today
One unrelated note while you're in here: isFormSheet is live now too, but nothing in the codebase sets presentation: 'formSheet' — the only occurrence is its own definition — so styles.buttonFormSheet is still untested dead code. Fine to leave, just worth knowing.
The gray700 rename looks right, and I'm treating the #545454 contrast as settled given Matt's confirmation. This gate is the only thing left from my side.
f2b5e9c to
8ecc841
Compare
…config - Use native canGoBack check in navigationStyle for back chevron - Add backButtonIcon token to themes for universal #545454 color - Remove duplicate withBackChevron from SendDetailsStack - Remove redundant headerBackButtonDisplayMode configs
8ecc841 to
4363735
Compare
theanmolsharma
left a comment
There was a problem hiding this comment.
The closeButton === CloseButtonPosition.None gate resolves it. Traced through every stack root:
SendDetails(None, first-route) — chevron restoredCloseButtonPosition.Right— gate skipped, no double dismissAddWallet/ReceiveDetails(Left) — gate skipped, X preservedWalletsList(true root) —canGoBack === false→null, no spurious chevron, andwalletListScreenOptionssets noheaderLeftto override itImportWallet— its ownsetOptionsheaderLeft still wins
Removing headerBackButtonDisplayMode: 'minimal' is a genuine no-op again, since headerLeft is now always set except for Right, which stays unreachable. gray700 sits correctly between gray500 and gray800, and no residual withBackChevron / gray980 references remain. tsc --noEmit and eslint are clean.
One finding is deliberately not blocking this: headerBackVisible: false can't suppress a custom headerLeft, so WalletExport gains a chevron it opted out of, and PleaseBackup has had the same problem since before this PR. Tracked in #139 with a one-line fix — worth settling soon given both are seed-phrase screens.
Thanks for fixing isModal at the root rather than just commenting it.
Changes