feat(icons): publish XmarkIcon and add the disabled menu-send-account variant - #12
Merged
Conversation
`XmarkIcon` was internal, so app code reaching for a close control had to
fall back to the bundled `navigationbar-close` asset. That asset is fixed
artwork: it neither scales cleanly to an arbitrary size nor takes a tint,
which is exactly what a close control drawn at 12pt inside a menu row
needs.
The icon is already a `Shape` with `size` / `color` / `lineWidth` knobs, so
nothing about it was internal by design — it simply had no reason to be
exported until now. Publishing it adds an explicit public initializer,
since a public struct gets no public memberwise init, and leaves the
call site unchanged for the module's own use in `Toast` and
`SystemMessageView`.
`docs/feedback.md` claimed the opposite ("Internal to the module ... reach
for the bundled navigationbar-close asset"), so it is corrected here and
now says when to prefer each.
|
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 ignored due to path filters (4)
📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesXmarkIcon public API
Menu icon case
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
`menu-send-account` had no disabled counterpart, so a `MenuItem` using it as its leading icon had nothing to pass for `disabledLeadingIcon` and kept the full-colour icon while the rest of the row was greyed out. Adds the imageset at the usual three scales (30/60/90) and maps it as `DashIcon.Menu.sendAccountDisabled`, following the existing `sendDisabled` / `receiveDisabled` pairs.
romchornyi
changed the title
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
3 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 icon gaps found while building a Join DashPay menu row in dashwallet-ios.
1.
menu-send-accounthad no disabled counterpart.MenuItemgreys out its title and help text whenisEnabledis false and swaps indisabledLeadingIcon— but with no disabled asset to pass, the row kept a full-colour blue icon against greyed text.2.
XmarkIconwasinternal. App code needing a close control had to fall back to the bundlednavigationbar-closeasset, which is fixed artwork: it does not scale cleanly to an arbitrary size and does not take a tint.What was done?
Disabled icon — adds the
menu-send-account-disabledimageset at the usual three scales (30/60/90) and maps it asDashIcon.Menu.sendAccountDisabled, following the existingsendDisabled/receiveDisabledpairs.XmarkIcon —
XmarkIconand its properties are nowpublic, with an explicitpublic init(size:color:lineWidth:)(a public struct gets no public memberwise initializer). Defaults are unchanged, so the module's own call sites inToastandSystemMessageVieware untouched — this is a visibility change only, with no behaviour or rendering difference.docs/feedback.mdstated the opposite ("Internal to the module (not part of the public API) — reach for the bundlednavigationbar-closeasset or an SF Symbol from app code") and is corrected to explain when to prefer the shape and when the navigation-bar artwork is still right.How Has This Been Tested?
swift buildon macOS — build complete, no new warnings. The new imageset is consumed byJoinDashPayMenuItemin dashwallet-ios#999 viadisabledLeadingIcon: .custom("menu-send-account-disabled", bundle: .dashUIKit), which is what surfaced the gap.Note for the reviewer
The new artwork is the same shape and hue as the existing disabled icons (
#B0B6BC) but is exported at 80% opacity, wheremenu-send-disabledandmenu-receive-disabledare fully opaque. It will therefore read slightly fainter next to them. Flagging rather than silently normalising it, in case the softer weight is intended — happy to re-export at 100% if not.Breaking Changes
None. Widening visibility cannot break existing callers, the added initializer matches the previous memberwise signature and defaults, and the new enum case is additive.
Checklist:
Summary by CodeRabbit
New Features
Documentation