refactor(ui): consolidate bottom sheets on DashUIKit - #1073
refactor(ui): consolidate bottom sheets on DashUIKit#1073PastaPastaPasta wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughRemoved the local ChangesBottomSheet migration
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR consolidates bottom-sheet behavior onto the shared DashUIKit implementation while preserving existing presentation and navigation behavior; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review\n\n---\n🤖 Posted autonomously by Codex on behalf of pasta. |
|
|
romchornyi
left a comment
romchornyi
left a comment
There was a problem hiding this comment.
No correctness bugs found — the migration is mechanically complete and semantically equivalent against the pinned DashUIKit revision (5b373b1), which is what I checked against rather than master.
What I verified:
- Every
BottomSheet(at HEAD isDashUIKit.BottomSheet(; no reference to the deletedBottomSheetHeightPreferenceKeyor a localselfSizingSheetsurvives. - All 14 files that now reference
DashUIKit.BottomSheetor.selfSizingSheet(...)carryimport DashUIKit, and the four newly-added imports introduce no member ambiguity — DashUIKit namespaces its tokens underFont.dash/Color.dashand exports none ofTextIntro,FeatureTopText,ButtonsGroup,IconName. - pbxproj has no dangling refs to the removed identifiers, and both targets that lost the files link the DashUIKit product.
- The one real code deletion — dropping the
BottomSheetHeightPreferenceKeypublication fromDashSpendConfirmationDialog— is safe. The production presenter goes through.selfSizingSheet(fallback: 500), which already bound to DashUIKit's implementation before this PR, and that one measures its wrapped content directly via aGeometryReaderbackground instead of reading the preference. The retained.fixedSize(horizontal: false, vertical: true)keeps the measurement decoupled from the offered detent, so there is no detent↔measurement feedback loop. - Chrome metrics are unchanged: DashUIKit's
NavigationBaris equivalent to the app's (minHeight: 64) and the grabber frame is still 18pt, so the hardcoded134 + rows * Ndetent math inTransactionFilterDialog.height/WalletSwitchDialog.heightstays correct..dashFont(.calloutMedium)adds only ~1pt of vertical metrics over.font(.calloutMedium), absorbed by the 64pt bar.
Three things worth acting on, all inline except the first:
The branch pin. Package.resolved isn't touched by this PR, so I can't comment inline. DashUIKit is pinned by branch (requirement = { branch = master }) with resolved revision 5b373b1. After this PR every bottom sheet in the app depends on that moving branch — and master has already diverged from the pin in ways that change sheet behaviour: BottomSheet now unconditionally applies .interactiveDismissDisabled(!isDismissalEnabled), and selfSizingSheet applies .presentationBackground(background) even when cornerRadius is nil (the pinned revision applies it only alongside a corner radius). Anyone who resolves packages to latest gets different rendering and different dismissal semantics on ~15 screens, with nothing in the PR to signal it. Worth bumping the pin deliberately here, or moving to a tag.
🤖 Reviewed with Claude Code
| var body: some View { | ||
| let showsTxDetailRoute = txDetailRoute != nil | ||
| let dialog = BottomSheet( | ||
| let dialog = DashUIKit.BottomSheet( |
There was a problem hiding this comment.
Flagging here as representative of all ~15 migrated sheets: the grabber changes colour app-wide.
The deleted local BottomSheet drew it with Color.dash.gray300Alpha50 — #B0B5BD at 50% alpha, with no dark-mode variant. DashUIKit.BottomSheet uses Color.dash.grabberFill — #B0B6BC at 100% alpha in light mode, white at 30% in dark.
So in light mode the grabber becomes twice as opaque on every bottom sheet in the app, and dark mode gains a variant it didn't have. This looks intentional (it aligns everything with DashSpendConfirmationDialog, which already used grabberFill), but the PR body says "Breaking Changes: None" and "preserved each migrated caller's existing … behavior", and there is no UI CI here — only Validate PR title and CodeRabbit ran. Two sheets are screenshotted; the change lands on all of them. Worth naming in the description and confirming with design.
| .ignoresSafeArea() | ||
| .sheet(isPresented: $isPresented) { | ||
| // let sheet = BottomSheet(showBackButton: .constant(false)) { | ||
| // let sheet = DashUIKit.BottomSheet(showBackButton: .constant(false)) { |
There was a problem hiding this comment.
Nit: this eight-line commented-out block was updated to the new name rather than deleted. Keeping dead code in sync with live code is pure carrying cost — it should go.
2 participants
This pull request was created by Codex.
Issue being fixed or feature implemented
Dash Wallet has been carrying a local copy of DashUIKit's bottom-sheet and self-sizing implementations. That duplicate has drifted from the package implementation.
What was done?
BottomSheetto the explicitly qualifiedDashUIKit.BottomSheetcomponent.DashUIKit.BottomSheetduplicated navigation chrome and reduced usable content space.BottomSheet.swiftandSelfSizingSheet.swiftsources and their target memberships.The package implementation now provides the shared grabber token, Dash font styling, window-aware self-sizing cap, direct geometry measurement, and hidden system drag indicator from one source of truth.
How Has This Been Tested?
dashpayscheme successfully from exact headab566189742eb386516e9fc4c2d9e940d577a749with Xcode on an iPhone 17 Pro simulator running iOS 26.5.be25f358d9f606103d4da956c1310d756d436070and full head using separate worktrees, separate DerivedData, and two clones of the same shut-down simulator fixture.git diff --check, simplification review, and the independent code-review quality gate with no findings.dashwalletscheme was also attempted during the initial migration; it currently stops on the pre-existing mainnet target-membership errors forTxDetailContactAvatarandTxDetailContactRowinTxDetailCells.swift, unrelated to this diff.Full-resolution evidence, exact revision/device provenance, dimensions, and SHA-256 hashes: f75a40a1af51c7608ff2d0550517ff47f8d44f3e
Transaction filter
Transaction detail
Breaking Changes
None. This consolidates the duplicated bottom-sheet implementation onto the currently pinned DashUIKit API while preserving purpose-built native sheets.
Checklist:
For repository code-owners and collaborators only