feat: let the host set the sheet background and badge the address field - #11
Open
romchornyi wants to merge 4 commits into
Open
feat: let the host set the sheet background and badge the address field#11romchornyi wants to merge 4 commits into
romchornyi wants to merge 4 commits into
Conversation
The fill was hardcoded to `primaryBackground` in five places, and the presentation background — the one that covers the home-indicator inset — was only applied when a `cornerRadius` happened to be passed. A host that wanted anything else had two bad options: restyle its own content and live with a pale strip along the bottom edge, or pass a corner radius it did not want purely for the side effect. `background` is now a parameter on `BottomSheet`, on the `selfSizing` factory and on `selfSizingSheet(…)`, defaulting to what it was. The presentation fill applies on iOS 16.4+ regardless of corner radius, so the strip matches whatever the sheet is filled with. Why the strip exists at all: the natural-height path deliberately measures without the bottom safe area, and `presentationDetents([.height])` adds that inset back — so it lies outside the sheet's own stack and needs the presentation background to be covered.
The label row was a bare Text, so a host with something to say about the address it decoded — which chain it turned out to belong to — had nowhere to put it and had to draw its own label row above the field, printing the label twice. An optional accessory sits opposite the label. It is deliberately not inside the field: the controls in there act on the address (paste, scan, clear), while this describes it. Generic over the accessory with an EmptyView convenience initializer, the same shape NavigationBar uses, so existing callers are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A generic type cannot hold static stored properties, so the nested Layout enum stopped compiling the moment the view gained its accessory parameter. SwiftPM accepted it; Xcode did not, which is where it surfaced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review. 📝 WalkthroughWalkthrough
ChangesUI component enhancements
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The changes add optional sheet background and address-field accessory customization while preserving existing call-site behavior; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Caller
participant BottomSheet
participant SelfSizingSheet
participant Presentation
Caller->>BottomSheet: Provide background color
BottomSheet->>SelfSizingSheet: Propagate background
SelfSizingSheet->>Presentation: Apply presentation background
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/DashUIKit/Components/BottomSheet.swift (1)
190-210: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestrict the presentation-background guarantee to iOS 16.4 and later.
presentationDetentssupports iOS 16.0, butpresentationBackgroundsupports iOS 16.4 and later. On iOS 16.0–16.3,selfSizingSheetdoes not fill the presentation’s bottom safe-area strip. Update the source documentation and the two statements indocs/navigation-and-containers.md, or add a UIKit fallback. The existing no-op path below iOS 16 satisfies the iOS 14 fallback requirement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/DashUIKit/Components/BottomSheet.swift` around lines 190 - 210, Restrict the presentation-background guarantee to iOS 16.4 and later: update the selfSizingSheet documentation in Sources/DashUIKit/Components/BottomSheet.swift and both affected statements in docs/navigation-and-containers.md at lines 98 and 112-116 to state that iOS 16.0–16.3 do not fill the bottom safe-area strip, while preserving the existing no-op behavior below iOS 16; no direct code change is required in the shown modifier branch unless adding a UIKit fallback instead.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/DashUIKit/Components/AddressFieldView.swift`:
- Around line 341-365: Wrap the iOS 17/macOS 14 `#Preview("Label accessory")`
declaration in `#if DEBUG` and `#endif`, preserving the existing preview
implementation unchanged.
- Around line 352-353: Replace the direct Image(systemName:) badge icon in
AddressFieldView with an appropriate DashIconSource, then render that source
using Image(dash: source) while preserving the existing size and weight styling.
- Around line 343-356: Localize the preview’s user-facing strings in
AddressFieldView by wrapping “Address,” “Dash address,” and “Transparent
address” with NSLocalizedString using bundle: .module and appropriate comments;
add matching entries to the module’s localization resources.
- Around line 31-32: Restore compatibility in AddressFieldView by isolating the
`@FocusState-dependent` behavior and text-field branches behind iOS 15/macOS 12
availability, and provide an equivalent iOS 14/macOS 11 fallback. Then update
AddressFieldView and its compatibility initializer availability to iOS 14/macOS
11 while preserving focus behavior on newer platforms.
In `@Sources/DashUIKit/Components/BottomSheet.swift`:
- Around line 179-188: The selfSizingSheet modifier’s default background must
match the enclosing BottomSheet background. Update the BottomSheet and
selfSizingSheet flow to propagate and reuse the configured sheet color via the
existing environment mechanism, or require the modifier caller to provide it
explicitly; remove the incorrect independent .dash.primaryBackground default and
preserve matching content and presentation colors.
- Around line 305-326: Wrap the `#Preview` declaration for “BottomSheet Custom
Background” in `#if` DEBUG and a matching `#endif`, preserving its existing iOS
17/macOS 14 availability and preview content.
- Around line 190-206: Update the macOS path in the availability-gated modifier
flow around SelfSizingSheetModifier so presentationBackground(background) is
applied on macOS 13.3 and later. Keep the existing iOS-specific corner-radius
handling intact, and ensure older macOS versions continue using the fallback
behavior.
---
Outside diff comments:
In `@Sources/DashUIKit/Components/BottomSheet.swift`:
- Around line 190-210: Restrict the presentation-background guarantee to iOS
16.4 and later: update the selfSizingSheet documentation in
Sources/DashUIKit/Components/BottomSheet.swift and both affected statements in
docs/navigation-and-containers.md at lines 98 and 112-116 to state that iOS
16.0–16.3 do not fill the bottom safe-area strip, while preserving the existing
no-op behavior below iOS 16; no direct code change is required in the shown
modifier branch unless adding a UIKit fallback instead.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 53993a54-173b-43e7-968e-c6816a0d573d
📒 Files selected for processing (3)
Sources/DashUIKit/Components/AddressFieldView.swiftSources/DashUIKit/Components/BottomSheet.swiftdocs/navigation-and-containers.md
- `selfSizingSheet` docs no longer claim the background defaults to the wrapped sheet's own — the modifier cannot see it, so a custom colour has to be passed here too (or go through `BottomSheet.selfSizing`). - Apply `presentationBackground` on macOS 13.3+ as well; the parameter was silently dropped there because only the iOS branch used it. - Wrap the `BottomSheet` previews in `#if DEBUG`, matching the rest of the library. - Badge preview uses `DashIcon.Common.iconDashCurrency` instead of an SF Symbol.
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
2 participants
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.
Issue being fixed or feature implemented
Two components could not be adapted by their hosts without forking them.
BottomSheethard-codedColor.dash.primaryBackgroundin three places, so asheet that belongs on a different surface had no way to say so. Restyling only
the content is not enough: the background is also what the presentation detent
paints behind the home-indicator inset, so a host that overrode its own content
still got a strip of the original colour along the bottom edge.
AddressFieldViewhad nowhere to put anything beside its label. The consumerin dashwallet-ios needs a badge there naming what an entered address decoded to
— a property of the field, not an action on it.
What was done?
BottomSheettakes abackground: Color, defaulting to.dash.primaryBackgroundso every existing call site renders exactly asbefore. It is applied to the sheet body, to both content branches, and to the
presentation background, so the bottom-edge strip cannot reappear.
AddressFieldViewbecame generic over anAccessoryview placed opposite thelabel, with a
Spacerbetween them. Callers that want none keep the shape theyalready had: a constrained extension on
Accessory == EmptyViewsupplies theold initializer verbatim, so no existing call site changes.
Its
Layoutconstants moved out of the type to file scope. A generic typecannot hold static stored properties — worth knowing that SwiftPM compiled the
nested version anyway and only Xcode rejected it, so
swift buildalone doesnot prove this file.
How Has This Been Tested?
swift buildclean, plus a fulldashpaybuild in dashwallet-ios pinned tothis branch — that consumer is the one exercising both new parameters, and it
is the build that catches what SwiftPM misses. Exercised on the simulator: the
address step with the destination badge, and the sheets that pass a background.
Existing call sites were not touched, and both defaults reproduce the previous
rendering.
Breaking Changes
None at call sites — both additions are defaulted, and the
EmptyViewextension keeps
AddressFieldView's original initializer.Worth flagging for anyone writing the type name explicitly:
AddressFieldViewis now generic, so an explicit annotation needs a parameter
(
AddressFieldView<EmptyView>). Inferred usage is unaffected.Checklist:
Summary by CodeRabbit
New Features
Documentation