Repository navigation
feat(native): Swift Avatar with core's picture, initials, and fallback order - #8306
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report
|
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the draft component:swift/Avatar requirements; an engineering owner records it by commenting /approve-spec 1b1add94683f5e8efec31fb078f56833f29aa7e9 on this pull request, and the author may do so as one. Two points need that decision because current spec says otherwise: FR3 keeps the avatar and its initials at fixed sizes while spec:AST-082 FR11 says text follows the text-size setting, and the Swift contract leaves out tooltip while spec:AST-082 FR3 keeps it in the core. Separately from that decision, these changes are needed:
- The link's
openURLoverride (Avatar.swift:66) replaces any action the app set above the avatar; install it only withonClickand forward to the inherited action. spec:AST-093FR5 asks for captures of every state (light, dark, largest text size) attached to the pull request, plus trait assertions and a largest-text-size test, as the other Swift views have.- On iOS 17 and later a malformed
hrefstill becomes a link, which contradicts the record's control row; accept only a URL with a scheme, or narrow the row.
[Automated review]
| SwiftUI.Link(destination: url) { picture } | ||
| .buttonStyle(.plain) | ||
| // `onClick` runs as the link opens, as core's does. | ||
| .environment(\.openURL, OpenURLAction { url in |
There was a problem hiding this comment.
This override replaces any openURL action the app set above the avatar, because SwiftUI calls only the closest one. In-app link routing therefore never sees an avatar tap, even when onClick is nil. Install the override only when onClick is set, and forward to the inherited action: read @Environment(\.openURL) in Avatar, call props.onClick?(), then call the inherited action, and return .handled.
|
|
||
| private func url(_ string: String?) -> URL? { | ||
| guard let string, !string.isEmpty else { return nil } | ||
| return URL(string: string) |
There was a problem hiding this comment.
On the iOS 17 and macOS 14 floor, URL(string:) percent-encodes invalid characters. href: "ada profile" therefore becomes the relative URL ada%20profile and a Link the system cannot open, while the record says an href that is not a URL leaves the avatar static. Accept only a URL with a scheme here, or narrow the record's row to an empty href; a test with a space would pin either choice.
1b1add9 to
1cc0347
Compare
812e3bc to
7d287bf
Compare
|
/approve-spec 7d287bf |
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the draft component:swift/Avatar requirements; an engineering owner records it by commenting /approve-spec 7d287bf9038f260800d6fc5c87fb9bc7d2266e16 on this pull request, and the author may do so as one. Three points need that decision because current spec or core's documented behavior says otherwise: FR3 keeps the avatar and its initials at fixed sizes while spec:AST-082 FR11 says text follows the text-size setting, the Swift contract leaves out tooltip while spec:AST-082 FR3 keeps it in the core, and AV1 loads fallbackSrc while src is still loading, where core uses it only after src fails. Separately from that decision, these changes are needed:
- The link's
openURLoverride (Avatar.swift:66) replaces any action the app set above the avatar; install it only withonClickand forward to the inherited action. spec:AST-093FR5 asks for trait assertions and a largest-text-size test, as the other Swift views have, and for largest-text captures of every state.- On iOS 17 and later a malformed
hrefstill becomes a link, which contradicts the record's control row; accept only a URL with a scheme, or narrow the row. - A link or button avatar without
altornamegets no development warning, which core documents for bothhrefandonClick.
[Automated review]
| SwiftUI.Link(destination: url) { picture } | ||
| .buttonStyle(.plain) | ||
| // `onClick` runs as the link opens, as core's does. | ||
| .environment(\.openURL, OpenURLAction { url in |
There was a problem hiding this comment.
This override replaces any openURL action the app set above the avatar, because setting the environment value replaces the action and only the closest one runs. In-app link routing therefore never sees an avatar tap, even when onClick is nil, and a relative href such as /users/john becomes a link that does nothing. Install the override only when onClick is set and forward to the inherited action: read @Environment(\.openURL) in Avatar, call props.onClick?(), then the inherited action, and return .handled.
| return .systemAction(url) | ||
| }) | ||
| .modifier(AvatarTarget(side: side)) | ||
| .modifier(AvatarName(label: accessibilityLabel, isInteractive: true)) |
There was a problem hiding this comment.
When neither alt nor name is set, this link (and the button below) gets no accessible name from the view and nothing warns the developer. Core documents that an interactive avatar without one "warns in development" for both href and onClick; a debug-only warning when linkURL or onClick is set and accessibilityLabel is nil would match it.
|
|
||
| private func url(_ string: String?) -> URL? { | ||
| guard let string, !string.isEmpty else { return nil } | ||
| return URL(string: string) |
There was a problem hiding this comment.
On the iOS 17 and macOS 14 floor, URL(string:) percent-encodes invalid characters instead of failing, so href: "ada profile" becomes the relative URL ada%20profile and a Link the system cannot open, while the record says an href that is not a URL leaves the avatar static. Accept only a URL with a scheme here, or narrow the record's row to an empty href; a test with a space would pin either choice.
d1cf302 to
a5c20e5
Compare
|
/approve-spec a5c20e5 |
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the draft component:swift/Avatar requirements at this head: the latest commit rewrote FR1, FR5 and AV1 and added AR4 after the previous head was approved, so an engineering owner records the decision by commenting /approve-spec a5c20e5f757fd79d0bb85e23a898100c02b26348 on this pull request, and the author may do so as one. Three points need that decision because current spec or core says otherwise: FR3 keeps the avatar and its initials at a fixed size while spec:AST-082 FR11 says text follows the text-size setting, the Swift contract leaves out tooltip while spec:AST-082 FR3 keeps it in the core, and FR5 leaves a relative href static where core renders every href as a link. Separately from that decision, these changes are needed:
spec:AST-093FR5 asks for captures of every state at the largest text size, but the largest-text captures end inside the fallback row and were built from an earlier head's Swift code.- The description should record the open-work search terms, as the knowledge-record contract asks when a record is created.
[Automated review]
a5c20e5 to
1b16264
Compare
|
/approve-spec 1b16264 |
There was a problem hiding this comment.
Changes requested. The exact-head approval settles the draft component:swift/Avatar record, but three of its choices still contradict current spec:AST-082, which a native spec cannot override (spec:AST-093 FR1). For each one, either change the view, or have an engineering owner amend spec:AST-082 on main first; the author may do that as one.
- FR3 keeps the initials at a fixed size (
Avatar.swift:156), but FR11 says text follows the text-size setting; at the largest text size, the name in the attached capture grows about threefold beside an unchanged avatar. - The Swift contract has no
tooltip, but FR3 keepstooltipin the core. - A relative
href, such as core's/users/john, is accepted and silently ignored (Avatar.swift:179), which FR3 rules out for a prop a platform offers. - The attached captures were built from an earlier
Avatar.swift, not this head: recapture them, or report the loading state and link handling as not measured.
[Automated review]
| private var initialsFont: Font { | ||
| astryxFont( | ||
| families: astryx.tokens.font.familyBody, | ||
| size: side * 0.4, |
There was a problem hiding this comment.
The initials never follow Dynamic Type: side * 0.4 reaches astryxFont without astryx.textScale, while spec:AST-082 FR11 says text follows the platform's text-size setting. Swift Icon multiplies its side by astryx.textScale, and doing the same here (for side, or at least the initials) would conform; otherwise FR11 needs amending on main first.
| var linkURL: URL? { | ||
| // Only an absolute URL the system can open; a malformed or relative | ||
| // `href` leaves the avatar static or a button. | ||
| guard let url = url(props.href), url.scheme != nil else { return nil } |
There was a problem hiding this comment.
A relative href, such as core's /users/john, is accepted here and silently ignored: the avatar stays static, or becomes a button that drops the href. spec:AST-082 FR3 rules out a prop a platform "accepts but ignores, or honors only in part". Making every non-empty href a Link lets the inherited openURL, which an app's router can set, resolve it.
|
|
||
| ## Open questions | ||
|
|
||
| - **OQ1 — How does `tooltip` reach Swift?** (`human-api`) `spec:AST-082` FR3 |
There was a problem hiding this comment.
spec:AST-082 FR3 keeps tooltip in the core, and this record cannot override it (spec:AST-093 FR1). Until FR3 or FR4 is amended on main, or tooltip gets a portable form honored with .help, the Swift contract is missing a core prop.
1b16264 to
cbe325b
Compare
|
/approve-spec cbe325b |
|
/approve-spec 98166e7 |
…ly absolute URLs, loads the fallback only after src fails, and flags unnamed taps
…cap, and stops a development build on a web-path href
…VoiceOver does not read the name twice
…ubble VoiceOver skips
…r or keyboard focus, and its spec lists the bubble's theme values
… touch screens, as core's touchTrigger does
…op, and a press hides its tooltip, as core's
…pointer, keyboard, and touch, with given words as the VoiceOver hint
…tes core's touchTrigger
…atar whose tap opens its tooltip answers across 44 points
…f, and the initials are never their own element
…ed-props views AstryxNodeView draws
…ility element of their own
62d9cf7 to
d33c512
Compare
|
/approve-spec d33c512 |
There was a problem hiding this comment.
Recommending approval: the Swift Avatar follows core's picture, initials, sizes, link, button and tooltip behaviour, the draft component:swift/Avatar record carries an engineering owner's approval at this head, and the Swift tests pass on macOS and the iOS Simulator. Nothing blocks; as follow-ups for the specs, spec:AST-082 FR4 could name the string | boolean form the generator now adds, and the Avatar or Swift accessibility record could say how a focused avatar shows focus and whether an avatar with tooltip words but no name should be a tab stop.
[Automated review]
User need
People lists, comment threads, account screens, and chat headers on iOS need a picture of who they are looking at. There is no Swift
Avatar, so an app hand-rolls one: its own sizes, its own initials rule, no fallback when the photo fails, and no name for VoiceOver. The person using the app sees avatars that don't match the web, blank circles when a photo fails, and VoiceOver users hear nothing or "image".Problem and solution fit
Avatarview. Core's fallback order (photo → fallback photo → initials → person symbol), core's initials rule (letters, digits, whole emoji; punctuation skipped), core's sizes (20/24/36/48/128 pt) and shapes (circle, the theme's element radius, square), painted with the theme's neutral plate and secondary text color.hrefit is a SwiftUILinkthe system opens; with onlyonClickit is a SwiftUIButton. It draws exactly as the static avatar (component API A4) and answers taps across 44×44 pt without growing its layout.sizeis'xsm' | … | 'xl' | 16 | 20 | … | 180, a mixed union the generator drops as web-only. AST-082 FR4 lets a wider web type name the closed set every platform supports; Avatar projectssizeto its five named sizes with the generator'sprojections, soAvatarProps.sizeisxsm–xland the pixel sizes stay on the web. Theprojectionsdoc comment no longer says the first value is the default, since Avatar's default ismd.Current specification
spec:AST-082FR3 (only props a platform honors;target/relare web), FR4 (portable closed set of a wider web type), FR7.spec:AST-093FR1–FR5.href→Link, Avatar has no disabled state), theme environment, accessibility (X1, X5, X7).component:swift/Avataratpackages/native/apple/Sources/Astryx/Avatar/Avatar.spec.md(draft,platform_of: component:Avatar).Public delta and caller burden
Avatar(Astryx); generatedAvatarProps,AvatarSize,AvatarShape(AstryxNative).tooltipis honored through the shared Astryx tooltip (feat(native): one internal Swift tooltip that follows core's useTooltip on pointer, keyboard, touch, and VoiceOver #8402):nilortrueshows the name, a string those words,falsenone. Itsstring | booleantype gets a portable form in the generator (textOrBoolean, a Swift enum that travels as the web value).status(needs a SwiftAvatarStatusDot; the spec keeps it as OQ2),target,rel. Anhrefmust be an absolute URL; a web path stops a development build with its value and draws without a link in release.AstryxNodeView(AST-082 FR19), and the component API record lists it.Design and alternatives
AsyncImage. The fallback photo loads only aftersrcfails, as core does; while a photo loads, the initials or symbol show instead of an empty plate.hrefleaves the avatar static or a button. WithonClick, the link runs it and then hands the URL to the app's own URL handler.altornamelogs a development warning, as core warns.person.fill.Evidence
Capture set: iPhone 16 simulator, iOS 18.5, Xcode 16.4, built from
ea6d4da4by a scratch capture app, with the initials drawn; the head since adds only@MainActorto one test. The tooltip on hover, keyboard focus, and tap does not change what an untouched avatar draws; an app-hosted UI test opens it with a tap and finds it above the avatar. Each capture shows sizes, shapes, the four fallbacks, emoji and flag initials, static/link/button avatars, a row beside text, and three themes.Not measured: the pointer and keyboard-focus tooltip on macOS and iPadOS, and the tap bubble on touch, including that a tap on an avatar inside a row still runs the row (the tap is a simultaneous gesture). The captures cannot hover or tap;
AvatarTests.swiftchecks which words show, when a tap opens the tooltip, and the bubble's drawing.check:knowledgedoes not scan native component specs yet; this record passes the checker in #8288, which does.Implementation choices under current rules
These carry out
spec:AST-082FR3, FR4, and FR11 as they stand; none sets a new rule:tooltipin the core contract (FR3).string | booleangets a portable form,textOrBoolean(FR4: every core prop has a form in each language), which travels as the web value.Open-work search
Searched current records and open pull requests before creating
component:swift/Avatar, by canonical id (component:swift/Avatar,component:Avatar), paths (packages/native/apple/Sources/Astryx/Avatar/,packages/native/src/components/Avatar.ts), symbols (Avatar,AvatarProps,projections,portableValues), and terms (native,swift,avatar). No other record or open pull request owns this platform component; the Swift views moved onto core names in #8299 and the Icon slot in #8302 are its neighbors.Scope
Writing rubric results
Testing
vitest run packages/native: the Avatar contract (named sizes projected, pixel sizes,status,target,relon the web) passes; the committed Swift binding matches the contracts.swiftc -typecheckofAstryxNative,Astryx, and the tests passes. An immediate-mode render checks core's initials cases, the 48 pt square, a clear circle corner, the plate in light, dark, and butter, that link and button draw the same as static, and that a hidden avatar draws nothing.Every changed file is under
packages/native/.