Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
6f63724 to
20321d9
Compare
20321d9 to
49056df
Compare
There was a problem hiding this comment.
This adds a Swift List (with the ListItem and Item it draws), and approval waits on an engineering owner deciding the new draft Swift records: component:swift/List FR1-FR6, including the padded/flush-only edge left open in its OQ1, and the carried component:swift/ListItem FR1-FR5 and component:swift/Item FR1-FR6, including swipe actions staying web-only, which no current spec states yet. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Separately from that decision, these need changes:
- A decimal list whose
startchanges while its rows keep their identity keeps its old numbers, because numbers are assigned only when the row preference changes (List.swiftlines 77-78). - A
ListItemplaced in another row's start or end content takes the outer list's density, marker, corners and inset, which draft ListItem FR4 forbids, becauseItemclears only its own treatment for slots (ListItem.swiftline 25). Int(_:)traps onlabelLines: .infinityor an out-of-rangestart(Item.swiftline 327,List.swiftline 113), so checkisFiniteand clamp first.- AST-093 FR5 asks for a two-theme render and accessibility assertions for List and ListItem, which their tests do not have yet.
[Automated review]
| .environment(\.astryxListContext, context) | ||
| .environment(\.astryxListNumbers, numbers) | ||
| .onPreferenceChange(ListRowsKey.self) { rows in | ||
| MainActor.assumeIsolated { numbers.assign(rows, start: firstNumber) } |
There was a problem hiding this comment.
Numbers are assigned only when ListRowsKey changes, and the rows' @State UUIDs stay the same when only start changes, so a decimal list re-rendered with a new start keeps its old numbers. Keeping the row order in ListNumbers and adding start when a row reads its number (or reassigning in .onChange(of: firstNumber)) fixes it; a test that re-renders with a new start would hold it.
| let startContent: StartContent | ||
| let endContent: EndContent | ||
| @Environment(\.astryx) private var astryx | ||
| @Environment(\.astryxListContext) private var list |
There was a problem hiding this comment.
Item clears only astryxItemListTreatment for slot content, but this row rebuilds its treatment from astryxListContext, so a ListItem in another row's start or end content takes the outer list's density, marker, corners and inset, which draft FR4 rules out. Clearing astryxListContext and astryxListNumbers on slot content, plus an FR4 test, would cover it.
| /// A line count from the contract's number: `nil` or less than one is one line. | ||
| nonisolated static func lines(_ value: Double?) -> Int { | ||
| guard let value, value >= 1 else { return 1 } | ||
| return Int(value) |
There was a problem hiding this comment.
Int(value) traps when value is infinite or above Int.max, so labelLines: .infinity crashes the app; the same applies to Int($0) for start in List.swift. Checking isFinite and clamping first keeps the fallback the records describe.
49056df to
09ac884
Compare
There was a problem hiding this comment.
This adds a Swift List (with the ListItem and Item it draws), and approval waits on an engineering owner deciding the three new draft Swift records: component:swift/List FR1-FR6, including the padded/flush-only edge left open in its OQ1, component:swift/ListItem FR1-FR5, and component:swift/Item FR1-FR6, including swipe actions staying web-only, which no current spec states yet. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Separately from that decision, these need changes:
List.tsdeclareschildrenwithout asameMeaning, so the generatedListNodecannot carry the list's rows (AST-082 FR19).- A decimal list whose
startchanges while its rows keep their identity keeps its old numbers (List.swiftlines 77-78). - The fixed 16-point number column lets numbers draw over their labels at accessibility text sizes (
ListItem.swiftline 141); scaling it with the text, as core's1remcolumn does, avoids that. Int(_:)traps onlabelLines: .infinityor an out-of-rangestart(Item.swiftline 327,List.swiftline 113).- List still lacks the two-theme render and accessibility assertions AST-093 FR5 asks for.
- The change carries #8321 and #8370 and so exceeds 2,000 authored lines; landing those first leaves this pull request as the List alone.
[Automated review]
| doc: {file: 'List/List'}, | ||
| baseTarget: 'root', | ||
| slots: { | ||
| children: 'content', |
There was a problem hiding this comment.
children has no sameMeaning here, so coreNameProblem reports that it "already means something else in web" and files it web-only; the generated ListNode then carries header but no rows, against AST-082 FR19 ("A content slot (FR4) carries child nodes"). Card, HStack, VStack, Text and Heading declare sameMeaning: {children: "the component's content, as on the web"}; adding it, regenerating, and asserting list.core.children in the List contract test fixes it.
| .onPreferenceChange(ListRowsKey.self) { rows in | ||
| MainActor.assumeIsolated { numbers.assign(rows, start: firstNumber) } |
There was a problem hiding this comment.
Numbers are assigned only when ListRowsKey changes, and the rows' @State UUIDs stay the same when only start changes, so a decimal list re-rendered with a new start keeps its old numbers. Keeping the row order in ListNumbers and adding start when a row reads its number (or reassigning in .onChange(of: firstNumber)) fixes it; a test that re-renders with a new start would hold it.
| .font(astryx.font(size: "text-body-size", 14, weight: "text-body-weight")) | ||
| .foregroundStyle(astryx.tokens.color.textPrimary) | ||
| .fixedSize() | ||
| .frame(width: astryx.tokens.space.s4, alignment: .leading) |
There was a problem hiding this comment.
The number grows with Dynamic Type (fontSize times textScale, 3.1 at the largest size) but this frame stays 16 points, and the label starts 8 points later. In the PR's own largest-text capture "3." needs about 30 points, so on this head it paints over the label's first letter. Scaling the column with the text (space.s4 * astryx.textScale, as core's 1rem column grows) keeps one text edge and clears the label; a largest-text decimal render would hold it.
| /// A line count from the contract's number: `nil` or less than one is one line. | ||
| nonisolated static func lines(_ value: Double?) -> Int { | ||
| guard let value, value >= 1 else { return 1 } | ||
| return Int(value) |
There was a problem hiding this comment.
Int(value) traps when value is infinite or above Int.max, so labelLines: .infinity stops the app; Int($0) for start in List.swift has the same problem. Checking isFinite and clamping first keeps the fallback the records describe.
a1487f3 to
71a769e
Compare
There was a problem hiding this comment.
This adds a Swift List (with the ListItem and Item it draws), and approval waits on an engineering owner deciding the three new draft Swift records: component:swift/List FR1-FR6, including the padded/flush-only edge left open in its OQ1, component:swift/ListItem FR1-FR5, and component:swift/Item FR1-FR6, including swipe actions staying web-only, which no current spec states yet. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. The fixes asked for on the earlier heads hold here; separately from the spec decision, these still need changes:
- The Swift list gives VoiceOver no name, while core names a list by its header (
aria-labelledbyinList.tsx); name the rows' container from the header and assert it, or record in List AR1 how reading the header first carries that name on Apple platforms (List.swiftlines 88-90). - The change carries #8321 and #8370 and so exceeds 2,000 authored lines, and no capture attached here shows their Item states (selected, highlighted, disabled, pressed); landing those first leaves this pull request as the 591-line List change.
[Automated review]
| // A list counts only its own rows, not those of a list around it. | ||
| .transformPreference(ListRowsKey.self) { $0 = [] } | ||
| // The rows stay separate elements; the header is read before them. | ||
| .accessibilityElement(children: .contain) |
There was a problem hiding this comment.
This container has no accessible name, while core names a list by its header (aria-labelledby in List.tsx; List.doc.mjs: "Provide a header to label the list and give context to screen readers"), which AST-082 FR3 keeps in the core contract. Giving the rows' container the header as its name, with a ListTests.swift assertion, would cover it; otherwise List AR1 should say how reading the header first carries that name on Apple platforms.
71a769e to
c8f4c9c
Compare
There was a problem hiding this comment.
This adds a Swift List (with the ListItem and Item it draws), and approval waits on an engineering owner deciding the three new draft Swift records, whose requirements no current spec states yet: component:swift/List FR1-FR6 and AR1 (including the padded/flush-only edge left open in its OQ1 and the header read first in place of a list name and role), component:swift/ListItem FR1-FR5, and component:swift/Item FR1-FR6 (including swipe actions staying web-only). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Separately from that decision, these need changes:
- The list divider is fixed at 1 point (
List.swiftline 163), while AST-093 FR4 asks for theme values and core draws it at the theme'sborder-width; readingtokens.size.borderWidth, asCarddoes, fixes it. - AST-093 FR1 gives each native component its own folder named for the core component, so
ListItem.swiftandListItem.spec.mdbelong inSources/Astryx/ListItem/unless that rule is amended. - The change carries #8321 and #8370 and so exceeds 2,000 authored lines, and its captures show no circle marker or Item states; landing those first leaves this pull request as the 593-line List change.
[Automated review]
| if let divider, index < rows.count - 1 { | ||
| Rectangle() | ||
| .fill(divider) | ||
| .frame(height: 1) |
There was a problem hiding this comment.
This divider is always 1 point, while core draws it at the theme's --border-width and AST-093 FR4 asks for every such value to come from the theme. Drawing it at astryx.tokens.size.borderWidth (as Card.swift reads border-width) keeps an app theme with 2-pixel borders the same on the web and on iPhone; FR1 and the record's theme values would name it too.
| schema_version: 5 | ||
| template_version: 8 | ||
| kind: component | ||
| id: component:swift/ListItem |
There was a problem hiding this comment.
AST-093 FR1 gives each native component its own folder named as the core component (Sources/Astryx/<Name>/<Name>.spec.md), which for ListItem is Sources/Astryx/ListItem/. Moving this record and ListItem.swift there, with the Package.swift exclude, follows it, unless an engineering owner amends FR1 for components core keeps in another component's folder.
| // own disabled state). | ||
| words | ||
| .frame(maxWidth: .infinity, alignment: .leading) | ||
| .onGeometryChange(for: CGFloat.self) { $0.size.height } action: { wordsHeight = $0 } |
There was a problem hiding this comment.
This measure runs on every row, while draft ListItem PR1 says only rows in a list with markers measure their words, and only a center-aligned marker row reads wordsHeight. Applying it when listTreatment?.markerOnFirstLine == true, or rewording PR1, would make them agree.
c8f4c9c to
bb802d4
Compare
There was a problem hiding this comment.
This adds a Swift List (with the ListItem and Item rows it draws), and approval waits on an engineering owner deciding the three new draft Swift records, whose requirements no current spec states yet: component:swift/List FR1-FR6 and AR1 (including the padded/flush-only edge left open in its OQ1 and the header read first in place of a list name and role), component:swift/ListItem FR1-FR5, and component:swift/Item FR1-FR6 (including swipe actions staying web-only). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Separately from that decision, these need changes:
- AST-093 FR1 gives each native component its own folder named for the core component, so
ListItem.swiftandListItem.spec.mdbelong inSources/Astryx/ListItem/unless that rule is amended. - The change carries #8321 and #8370 and so exceeds 2,000 authored lines; landing those first leaves this pull request as the 607-line List change.
- AST-093 FR5 asks for captures of every state, and none attached here shows the
circlemarker or the rows' selected, highlighted, disabled or pressed states.
[Automated review]
bb802d4 to
afaf4ee
Compare
There was a problem hiding this comment.
This adds a Swift List (with the ListItem and Item rows it draws), and approval waits on an engineering owner deciding the three new draft Swift records, whose requirements no current spec states yet: component:swift/List FR1-FR6 and AR1 (including the padded/flush-only edge left open in its OQ1 and the header read first in place of a list name and role), component:swift/ListItem FR1-FR5, and component:swift/Item FR1-FR6 (including swipe actions staying web-only). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Separately from that decision, these need changes:
AstryxNodeViewdraws a wireItemorListItemfrom its props alone, so the nodes instartContent,endContentandmarkerare dropped, while AST-082 FR19 says a content slot carries child nodes; passing them throughAstryxNodeList, asCarddoes, fixes it.- AST-093 FR1 gives each native component its own folder named for the core component, so
ListItem.swiftandListItem.spec.mdbelong inSources/Astryx/ListItem/unless that rule is amended. - The change carries #8321 and #8370 and so exceeds 2,000 authored lines; landing those first leaves this pull request as the 608-line List change.
- The
Listdoc example'sHeading("Steps", level: 3)does not compile against the generatedHeadingLevel;level: .v3does.
[Automated review]
| case .heading(let heading): Heading(heading.viewProps(send: send)) | ||
| case .card(let card): Card(card.viewProps(send: send), children: card.props.children ?? [], send: send) | ||
| case .icon(let icon): Icon(icon.viewProps(send: send)) | ||
| case .item(let item): Item(item.viewProps(send: send)) |
There was a problem hiding this comment.
ItemNode.Props decodes marker, startContent and endContent as [AstryxNode], but this call passes only the props, so a logic core's icon or chevron in a row's slots is neither drawn nor shown as unsupported, against AST-082 FR19 ("A content slot (FR4) carries child nodes"). Passing each slot as AstryxNodeList(nodes: item.props.startContent ?? [], send: send), as Card does with its children, and the same for .listItem on the next line, keeps the slot content.
| schema_version: 5 | ||
| template_version: 8 | ||
| kind: component | ||
| id: component:swift/ListItem |
There was a problem hiding this comment.
AST-093 FR1 gives each native component its own folder named as the core component, with the spec at Sources/Astryx/<Name>/<Name>.spec.md where <Name> is the core component's name, so this record and ListItem.swift belong in Sources/Astryx/ListItem/ (with the Package.swift exclude), unless an engineering owner amends FR1.
| /// ListItem("Mix the batter") | ||
| /// ListItem("Bake for 30 minutes") | ||
| /// } header: { | ||
| /// Heading("Steps", level: 3) |
There was a problem hiding this comment.
Heading takes the generated HeadingLevel enum, which has no integer-literal conformance, so this example does not compile; Heading("Steps", level: .v3) does.
afaf4ee to
5d98803
Compare
There was a problem hiding this comment.
Approval of this Swift List waits on an engineering owner's decision on the three new draft records (component:swift/List, component:swift/ListItem and component:swift/Item), whose requirements no current spec states yet: the padded/flush-only edge, the header read in place of a list name and role, the fixed number column, the whole-row link and swipe actions staying web-only. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; the earlier approval named an older head. Separately from that decision, these need changes:
AstryxNodeViewdraws a wireListItemfrom its props alone and drops the child nodes in itsstartContentandendContentslots, which AST-082 FR19 says a content slot carries; theItemcase already passes them.ItemandListrepeatastryxBasefromBase/BaseProps.swiftforisHiddenandid; calling it keeps one path for base props.- AST-093 FR1 puts a component's views in
Sources/Astryx/<Name>/, soListItem.swiftbelongs inSources/Astryx/ListItem/unless that rule is amended. - At 2,310 authored lines the change carries #8321 and #8370; landing those first leaves the List commit alone, and tests for a hidden
ListorListItem,idmapping and a wireListItemslot, pluslevel: .v3in the doc example, close the rest.
[Automated review]
| } marker: { | ||
| AstryxNodeList(nodes: item.props.marker ?? [], send: send) | ||
| } | ||
| case .listItem(let row): ListItem(row.viewProps(send: send)) |
There was a problem hiding this comment.
ListItemNode.Props decodes startContent and endContent (Generated/ListItem.swift:34-35), but this case draws from props alone, so a wire row's icon or chevron is neither drawn nor marked unsupported (AST-082 FR19: "A content slot (FR4) carries child nodes"). Passing both through AstryxNodeList, as the Item case above does, and a wire test with a slot child would close it.
| } | ||
|
|
||
| /// The base `id` becomes the accessibility identifier (AST-082 FR7). | ||
| private struct ItemIdentifier: ViewModifier { |
There was a problem hiding this comment.
This repeats astryxIdentifier(_:) / astryxBase(_:) in Base/BaseProps.swift:7-20, which Button, Card, Heading and Text call; ListIdentifier in List/List.swift:176-186 is the same copy. Ending each body with .astryxBase(props.base) keeps one path for base props.
| // own disabled state). | ||
| words | ||
| .frame(maxWidth: .infinity, alignment: .leading) | ||
| .onGeometryChange(for: CGFloat.self) { $0.size.height } action: { wordsHeight = $0 } |
There was a problem hiding this comment.
Every row measures and stores wordsHeight, while draft ListItem PR1 says only rows in a list with markers measure, and only a center-aligned marker row reads it (:192). Measuring only when listTreatment?.markerOnFirstLine == true, or rewording PR1, makes them agree.
| /// ListItem("Mix the batter") | ||
| /// ListItem("Bake for 30 minutes") | ||
| /// } header: { | ||
| /// Heading("Steps", level: 3) |
There was a problem hiding this comment.
Heading's labeled initializer takes the generated HeadingLevel, an Int-backed enum with no integer-literal conformance, so level: 3 does not compile; level: .v3 does. The description's usage block repeats it.
| /// Icon(.check) | ||
| /// } onClick: { openWiFi() } | ||
| /// ``` | ||
| public struct ListItem<StartContent: View, EndContent: View>: View { |
There was a problem hiding this comment.
AST-093 FR1 puts a component's views in Sources/Astryx/<Name>/, where <Name> is the core component's name, so this view belongs in Sources/Astryx/ListItem/ (with the README line), unless FR1 is amended.
| types, unprefixed: `ButtonProps`, `ButtonVariant`). | ||
| - Avatar, AvatarStatusDot, Button, Card, Heading, Icon, Text, TextInput, and | ||
| Thumbnail take their generated `<Name>Props`; Banner has no generated | ||
| - Avatar, AvatarStatusDot, Button, Card, Heading, Icon, Item, ListItem, Text, TextInput, and Thumbnail take their generated `<Name>Props`; Banner has no generated |
There was a problem hiding this comment.
List also takes its generated ListProps (List/List.swift), so it belongs in this line too.
680f4bc to
084db32
Compare
There was a problem hiding this comment.
This adds a Swift List that spaces, divides and numbers ListItem rows like core's list, and approval waits on an engineering owner's decision on the new draft component:swift/List record. Two of its rules differ from spec:AST-082 FR3, which keeps a component's props and accessibility semantics the same on every platform: edge offers only padded and flush, and the header is read first instead of core's list name and role, so an owner either amends that spec (or component:List) or the Swift list conforms. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner.
List's doc exampleHeading("Steps", level: 3)does not compile;level: .v3does.packages/native/specs/swift/library/component-api.mdline 157 should listListamong the views that take their generated props.
[Automated review]
| /// ListItem("Mix the batter") | ||
| /// ListItem("Bake for 30 minutes") | ||
| /// } header: { | ||
| /// Heading("Steps", level: 3) |
There was a problem hiding this comment.
Heading's labeled initializer takes the generated HeadingLevel, an Int-backed enum with no integer-literal conformance, so level: 3 does not compile; level: .v3 does. The pull request description repeats the example.
| @@ -156,7 +156,7 @@ same generated contract. | |||
| types, unprefixed: `ButtonProps`, `ButtonVariant`). | |||
| - Avatar, AvatarStatusDot, Button, Card, Heading, Icon, Item, ListItem, Text, TextInput, and Thumbnail take their generated `<Name>Props`; Banner has no generated | |||
There was a problem hiding this comment.
List also takes its generated ListProps (List/List.swift:59), so it belongs in this line too.
084db32 to
dc4b2bd
Compare
There was a problem hiding this comment.
This adds a Swift List that spaces, divides and numbers ListItem rows like core's list, with no code defect found, and approval waits on an engineering owner's decision on the new draft component:swift/List record. Two of its rules differ from spec:AST-082 FR3, which keeps a component's props and accessibility semantics the same on every platform: edge offers only padded and flush, and the header is read first instead of core's list name and role. An owner either amends that spec (or component:List) or asks for the Swift list to conform. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner.
[Automated review]
dc4b2bd to
d4902f1
Compare
There was a problem hiding this comment.
Approval of this Swift List waits on an engineering owner's decision on the draft component:swift/List record: whether to approve FR1–FR6 and AR1, and how to settle its two departures from spec:AST-082 FR3 (edge offered with only padded and flush, and no list role where core's list sets one), by amending AST-082 or component:List or by conforming the Swift list. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Separately, the description and the record say VoiceOver names the list by its header, but theHeaderNamesTheList only checks the header's type and nothing shows the announced name, so please describe it as not measured (as AR2 does) or add evidence that shows it.
[Automated review]
| - **AR1 — The header names the list; rows stay separate.** As core names the | ||
| list by its header (`aria-labelledby`), the header MUST be the label of the | ||
| container that holds the rows (`accessibilityLabeledPair`), so VoiceOver | ||
| names the list by it; the rows stay separate elements, the header is still | ||
| read before them, and dividers are hidden (Swift accessibility X5). |
There was a problem hiding this comment.
"so VoiceOver names the list by it" states a rendered result nothing at this head shows: theHeaderNamesTheList checks only Header.self != EmptyView.self and would still pass without the two accessibilityLabeledPair calls, and AR2 says the rendered tree is not measured. Saying the header is paired as the label of the rows' container, with the announced name not measured, keeps AR1 and the verification row at line 155 within what the tests prove.
| // AR1: the header names the list, as core's aria-labelledby does: a list with | ||
| // a header pairs it as the label of its rows' container; one without labels | ||
| // nothing. The rendered accessibility tree is not measured here (AR2). | ||
| @Test func theHeaderNamesTheList() { |
There was a problem hiding this comment.
This checks the type-level namesRowsFromHeader predicate, not a render or the pairing itself, so the description's "an immediate-mode render checks … the header labelling the rows" overstates it.
d4902f1 to
f631e1b
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the draft component:swift/List record: whether to approve FR1–FR6 and AR1 as the Swift contract, and how to settle its two departures from spec:AST-082 FR3 (only the padded and flush edges, and no list role), by amending AST-082 or the core List record or by conforming the Swift list. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Separately, please bring the remaining VoiceOver wording in line with what the tests show:
- The record's End-user impact row and the
Listdoc comment still say VoiceOver names the list by its header, which AR2 reports as not measured. - The AR1 verification row and the description's Testing line describe a check
theHeaderNamesTheListcannot make, since it reads only a static type.
[Automated review]
| | ----------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Public contract | `List(density:, hasDividers:, edge:, listStyle:, start:, content:, header:)` and `List(_ props: ListProps, content:, header:)`, a generic SwiftUI `View`. | | ||
| | Behavior | Stacks its rows top to bottom with core's gap or a divider between rows, and tells each `ListItem` the list's density, marker, dividers, and edge; numbers the `ListItem`s of a decimal list in order from `start`. | | ||
| | End-user impact | A person sees settings lists, bulleted and numbered lists, and divided lists on iPhone spaced and numbered as on the web, and VoiceOver names the list by its header and reads each row. | |
There was a problem hiding this comment.
This row still says VoiceOver names the list by its header, while AR2 reports the announced name as not measured and theHeaderNamesTheList checks only Header.self != EmptyView.self. Saying VoiceOver reads the header before the rows, with the announced list name not measured (AR2), keeps the impact within what the tests show.
| | Contract | Verification | Representative states | Mutation or failure expectation | Audit section | | ||
| | -------- | ------------------------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------- | -------------------------------- | | ||
| | FR1 | `ListTests.swift` spacing, divider, two-theme, and largest-text renders; hidden-divider test | spaced; divided; neutral and butter; the largest text size | a wrong gap or divider color, a divider after the last row, a read divider, or a fixed row height fails | `audit:swift/List/layout` | | ||
| | AR1 | `ListTests.swift` header-pairing test (what the view sets; the announced name is not measured, AR2); hidden-divider test | with and without a header | a list with a header that does not pair it with the rows, a list without a header that pairs one, or a read divider fails | `audit:swift/List/accessibility` | |
There was a problem hiding this comment.
theHeaderNamesTheList reads only the static namesRowsFromHeader, so it still passes with both accessibilityLabeledPair calls removed and cannot make "a list with a header that does not pair it with the rows" fail. Either narrow this row to what the test checks (a header is paired only when one is given) or add a check that observes the pairing.
| /// Pairs the header with the rows' container, so VoiceOver names the list | ||
| /// by its header, as core's `aria-labelledby` does. |
There was a problem hiding this comment.
"so VoiceOver names the list by its header" states an outcome AR2 reports as not measured; saying the header becomes the label of the rows' container, as core's aria-labelledby names the list, describes what the view sets.
f631e1b to
de6eac7
Compare
There was a problem hiding this comment.
Approval of this Swift List waits on an engineering owner's decision on the draft component:swift/List record: whether to approve FR1–FR6 and AR1 as the Swift contract, and how to settle its two departures from spec:AST-082 FR3, the edge offered with only padded and flush and the missing list role, by amending AST-082 or component:List or by conforming the Swift list. The VoiceOver wording now matches what the tests show, and no other change is needed. An engineering owner, including the author if they are one, records the decision by commenting /approve-spec <full head SHA> on this pull request, or asks for the record or the code to change first.
[Automated review]
|
/approve-spec de6eac7 |
There was a problem hiding this comment.
The engineering-owner approval settles the Swift List record itself, but two parts of it still conflict with the native-first surfaces spec (spec:AST-082), which this pull request leaves unchanged: edge offers only padded and flush although core's contract has four values (FR3; FR4 allows narrowing only for wider-than-portable types), and the Swift list sets no list role where core sets role="list" (FR3). Please either add an owner-approved AST-082 change that admits both gaps, or offer outset and bleed as component:List FR6 defines them where no container publishes an inset (placed as padded, with square bleed rows) and amend AST-082 for the role alone. The layout, numbering, wire support, generated bindings and tests otherwise check out.
[Automated review]
| sameMeaning: {children: "the list's rows, as on the web"}, | ||
| // `outset` and `bleed` reach a container's padding, which Swift | ||
| // containers do not publish yet. | ||
| projections: {edge: {padded: 'padded', flush: 'flush'}}, |
There was a problem hiding this comment.
edge is a closed string set, so AST-082 FR4's projection (for a web type wider than the portable kinds) does not cover narrowing it, and the native core contract then differs from the web's (AST-082 FR3). component:List FR6 already defines outset and bleed where no host inset is published (placed as padded, bleed rows square), so Swift could offer all four values, or AST-082 needs an owner-approved amendment for this gap.
de6eac7 to
9252500
Compare
There was a problem hiding this comment.
Approval of this Swift List waits on an engineering owner's decision: the draft component:swift/List and component:swift/ListItem records changed after the last /approve-spec, which named an earlier commit, and the list still sets no list role although spec:AST-082 FR3 puts the role in the core contract on every platform. An engineering owner, including the author if they are one, records the decision by commenting /approve-spec <full head SHA>, and settles the role either with an AST-082 amendment in this pull request or by asking for a Swift list that sets it. Separately, two things need fixing:
- The
lintcheck fails onpackages/native/src/components/List.ts:25; thatedgeprojection maps each value to itself and can be removed, which also fixes the formatting. - No capture shows the new
bleedrows (square corners) in light, dark and the largest text size; please attach one or list it as not measured.
The outset and bleed values now follow the core List's placement when no container inset is published.
[Automated review]
| // No Swift container publishes its inset yet, so `outset` and `bleed` | ||
| // resolve the missing inset to zero (component:List FR6): rows sit as | ||
| // `padded` does, and `bleed` rows are square. | ||
| projections: {edge: {padded: 'padded', flush: 'flush', outset: 'outset', bleed: 'bleed'}}, |
There was a problem hiding this comment.
This line fails the required lint check (Prettier wants it wrapped). It also maps each edge value to itself, and without it the generator still puts edge in the core with the same four values; a projection is meant for a web type wider than the portable kinds (spec:AST-082 FR4). Deleting lines 22-25 fixes both.
| list by its header (`aria-labelledby`), the header MUST be the label of the | ||
| container that holds the rows (`accessibilityLabeledPair`); the rows stay | ||
| separate elements, the header is still read before them, and dividers are | ||
| hidden (Swift accessibility X5). Core's list role is not offered yet, an |
There was a problem hiding this comment.
spec:AST-082 FR3 puts accessibility semantics, role included, in the core contract "the same on every platform that offers the component", and it has no implementation-gap exception; spec:AST-093 FR1 says AST-082 wins over a native spec. Leaving the role out needs an owner-approved AST-082 amendment in this pull request (FR3, or its Current-state impact), or the Swift list needs to set the role.
9252500 to
78f451b
Compare
There was a problem hiding this comment.
Approval of this Swift List waits on an engineering owner's decision: the draft component:swift/List and component:swift/ListItem records have no approval at this head (the last /approve-spec named an earlier commit), and two parts of the Swift list leave the core contract that spec:AST-082 FR3 keeps the same on every platform. An engineering owner, including the author if they are one, records approval by commenting /approve-spec <full head SHA>, and settles both departures either with an AST-082 amendment in this pull request or by asking for a Swift list that conforms:
- The list sets no list role where core sets
role="list"; List AR1 calls it an implementation gap, but FR3 has no such exception. edgeCompensationis left out of Swift, though Swift can honor it asoutsetand FR3 lets a prop leave the core only when a platform cannot match it.
The earlier lint failure is fixed, and the bleed corners are now listed as not measured.
[Automated review]
| container that holds the rows (`accessibilityLabeledPair`); the rows stay | ||
| separate elements, the header is still read before them, and dividers are | ||
| hidden (Swift accessibility X5). Core's list role is not offered yet, an | ||
| implementation gap under `spec:AST-082` FR3: SwiftUI has no public API for a |
There was a problem hiding this comment.
spec:AST-082 FR3 puts accessibility semantics, role included, in the core contract "the same on every platform that offers the component", and it has no implementation-gap exception; spec:AST-093 FR1 says AST-082 wins over a native spec. Leaving the role out needs an owner-approved AST-082 amendment in this pull request (FR3, or its Current-state impact), or a Swift list that sets the role. The rationale here also does not say whether accessibilityRepresentation was tried.
| }, | ||
| sameMeaning: {children: "the list's rows, as on the web"}, | ||
| webOnly: { | ||
| edgeCompensation: 'deprecated on the web; `edge` replaces it', |
There was a problem hiding this comment.
AST-082 FR3 lets a prop leave the core only when a platform cannot match core's defined behavior, and Swift can honor edgeCompensation: 'inline' as it honors edge: .outset. Either the same AST-082 amendment that covers the list role says a deprecated alias stays out of a new platform's contract, or Swift offers edgeCompensation as the alias of outset.
78f451b to
a1c8efb
Compare
There was a problem hiding this comment.
Approval of this Swift List waits on an engineering owner's decision: the draft component:swift/List and component:swift/ListItem records have no approval at this head, and the list still sets no list role although spec:AST-082 FR3 keeps the role in the core contract on every platform. An engineering owner, including the author if they are one, records approval by commenting /approve-spec <full head SHA> on this pull request, and settles the role either with an AST-082 amendment in this pull request or by asking for a Swift list that sets it. That approval also covers the new FR6 rule that the labeled initializer leaves out the deprecated edgeCompensation, which now reaches Swift through ListProps and the wire as core resolves it.
[Automated review]
| container that holds the rows (`accessibilityLabeledPair`); the rows stay | ||
| separate elements, the header is still read before them, and dividers are | ||
| hidden (Swift accessibility X5). Core's list role is not offered yet, an | ||
| implementation gap under `spec:AST-082` FR3: SwiftUI has no public API for a |
There was a problem hiding this comment.
spec:AST-082 FR3 puts accessibility semantics, role included, in the core contract "the same on every platform that offers the component", with no implementation-gap exception, and spec:AST-093 FR1 says AST-082 wins over a native spec. Leaving the role out needs an owner-approved AST-082 amendment in this pull request (FR3, or its Current-state impact), or a Swift list that sets the role.
User need
Settings screens, bulleted and numbered lists, and divided lists sit inside cards, sheets, and scroll views on iOS. There is no Swift
Listthat matches core:SwiftUI.Listis a scrolling table with its own row styles that can't sit inside a card. Each app spaces, divides, and numbers rows by hand, and people get lists that differ from the web and from each other.Problem and solution fit
Listview. Core's list: a stack of rows with core's gap, or a divider between rows, that tells eachListItemits density, marker, dividers, and edge, in any container.decimallist numbers itsListItemrows in order fromstart, rows fromForEachincluded; a nested list counts apart.aria-labelledbynames the list, and read before the rows.component:ListFR6 defines:outsetandbleedrows sit aspaddedrows do, andbleedrows are square.Current specification
spec:AST-082FR3, FR4, FR7, FR19.spec:AST-093FR1–FR5.component:swift/ListItem, which draws each row; swipe actions stay web-only for now (component:swift/ItemDEC-1).component:swift/Listatdocs/specs/components/List/List.swift.spec.md(draft,platform_of: component:List), beside core's List record; its views inSources/Astryx/List/(AST-093 FR1).Public delta and caller burden
List(Astryx) and generatedListProps,ListDensity,ListEdge,ListListStyle(AstryxNative).Listnames Astryx's list;SwiftUI.Liststays available by its full name.edgeCompensation, which core deprecates, is carried byListPropsand the wire and resolves as core's List FR1 does (a definededgewins;inlinealone meansoutset); the labeled initializer omits it (FR6).…Styleprop whose type is a closed set of words, likelistStyle, is a semantic choice and stays in the core contract; style objects and class names stay on the web. No other generated file changes.Design and alternatives
SwiftUI.Listdoes not fit: it scrolls and reuses rows with its own styles, while core's list sits in any container (AST-093 FR2).Group(subviews:)on iOS 18 and macOS 15 and later. iOS 17 and macOS 14, which the package still supports, have no public API for a container's children, so only there the list uses SwiftUI's_VariadicView, whichGroup(subviews:)replaced (FR5). Both lay rows out the same way, a test covers the iOS 17 path, and the fallback goes away when the package's minimum moves to iOS 18.id,isHidden) go through the shared base-prop mapping, as every other view's do.Evidence
Capture set: iPhone 16 simulator, iOS 18.5, Xcode 16.4, built before the rebase onto the new spec layout and ListItem's number alignment; the wire list, hidden list, and number alignment are covered by tests. Each capture shows spaced and divided lists, a list numbered from 3, rows from
ForEach, a circle-marker list, selected and disabled rows between dividers, nested lists counting apart, a header, and a flush list inside a padded container, in light and dark at the default and largest text sizes.Not measured: a pressed row, which a capture cannot press, and
bleedrows' square corners in light, dark, and the largest text size, which the captures predate; a ListItem render test covers the bleed corner. ListItem has no highlighted state in its contract.check:knowledgepasses with the record at its new path.Implementation choices under current rules
aria-labelledby). The Swift list pairs the header as the label of the container that holds the rows (accessibilityLabeledPair), and the header is still read first (List AR1). The name VoiceOver announces is not measured yet: tests check the pairing the view sets, and the rendered tree waits on an app-hosted test (AR2). Core's list role is recorded as an AST-082 FR3 implementation gap (AR1): SwiftUI has no public API for a container's accessibility role, and UIKit's and AppKit's list roles belong to platform views, so offering them would mean hosting the rows in a UIKit or AppKit view.Open-work search
Searched current records and open pull requests before creating
component:swift/List, by canonical id (component:swift/List,component:List), paths (packages/native/apple/Sources/Astryx/List/,packages/native/src/components/List.ts), symbols (List,ListProps,ListRows,listStyle), and terms (native,swift,list). No other record or open pull request owns this platform component.Scope
Writing rubric results
SwiftUI.List, which AST-093 FR2 requires the spec to name.Testing
vitest run packages/native/src/contract: the List contract (all four edges andedgeCompensation,listStylein the core, content header;xstyleon the web) passes; the committed Swift binding matches the contracts.swiftc -typecheckofAstryxNative,Astryx, and the tests passes. An immediate-mode render checks numbering from 3, rows fromForEach, nested lists counting apart, the gap and the single divider between rows, the iOS 17 path, flush rows, the header gap, which lists pair a header with their rows, read from the header's type (the pairing modifiers and the announced name are not measured),outsetandbleedsitting aspaddedwith squarebleedrows,edgeCompensationresolving as core's, rows growing at the largest text size, a hidden list taking no space, and a wire list drawing its rows and header.Every changed file is under
packages/native/, plus the new record indocs/specs/components/List/.