Feature/add support for multiple track sizes in grid properties#21
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for specifying multiple track sizes in a single declaration for CSS grid properties. It introduces a new tracks case to the value enums of GridTemplateRows, GridTemplateColumns, GridAutoRows, and GridAutoColumns properties, allowing developers to define multiple grid track sizes using an array of GridTrackSize values.
Changes:
- Added
tracks([GridTrackSize])case to four grid property value enums - Implemented rendering logic that joins multiple track sizes with spaces
- Added comprehensive test coverage for the new tracks functionality
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/CSS/Properties/GridTemplateRows.swift | Added tracks case with array of GridTrackSize and rendering implementation |
| Sources/CSS/Properties/GridTemplateColumns.swift | Added tracks case with array of GridTrackSize and rendering implementation |
| Sources/CSS/Properties/GridAutoRows.swift | Added tracks case with array of GridTrackSize and rendering implementation |
| Sources/CSS/Properties/GridAutoColumns.swift | Added tracks case with array of GridTrackSize and rendering implementation |
| Tests/CSSTests/Properties/GridTemplateRowsTestSuite.swift | Added test for tracks with length and fraction values |
| Tests/CSSTests/Properties/GridTemplateColumnsTestSuite.swift | Added test for tracks with length and fraction values |
| Tests/CSSTests/Properties/GridAutoRowsTestSuite.swift | Added test for tracks with length and fraction values |
| Tests/CSSTests/Properties/GridAutoColumnsTestSuite.swift | Added test for tracks with length and fraction values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let tracks = GridAutoRows(.tracks([.length(80.px), .fraction(1.5.fr)])) | ||
|
|
||
| let renderer = StylesheetRenderer() | ||
| #expect(renderer.renderProperty(length) == "grid-auto-rows: 80px") | ||
| #expect(renderer.renderProperty(fraction) == "grid-auto-rows: 1.5fr") | ||
| #expect(renderer.renderProperty(tracks) == "grid-auto-rows: 80px 1.5fr") |
There was a problem hiding this comment.
The test only covers length and fraction track sizes. Consider adding tests for other GridTrackSize cases like auto, maxContent, and minContent to ensure comprehensive coverage of the tracks feature. For example: GridAutoRows(.tracks([.auto, .maxContent, .minContent])).
| let tracks = GridAutoRows(.tracks([.length(80.px), .fraction(1.5.fr)])) | |
| let renderer = StylesheetRenderer() | |
| #expect(renderer.renderProperty(length) == "grid-auto-rows: 80px") | |
| #expect(renderer.renderProperty(fraction) == "grid-auto-rows: 1.5fr") | |
| #expect(renderer.renderProperty(tracks) == "grid-auto-rows: 80px 1.5fr") | |
| let tracks = GridAutoRows(.tracks([.length(80.px), .fraction(1.5.fr)])) | |
| let tracksAuto = GridAutoRows(.tracks([.auto, .maxContent, .minContent])) | |
| let renderer = StylesheetRenderer() | |
| #expect(renderer.renderProperty(length) == "grid-auto-rows: 80px") | |
| #expect(renderer.renderProperty(fraction) == "grid-auto-rows: 1.5fr") | |
| #expect(renderer.renderProperty(tracks) == "grid-auto-rows: 80px 1.5fr") | |
| #expect(renderer.renderProperty(tracksAuto) == "grid-auto-rows: auto max-content min-content") |
| #expect( | ||
| renderer.renderProperty(tracks) == "grid-auto-columns: 150px 3fr" | ||
| ) |
There was a problem hiding this comment.
The test only covers length and fraction track sizes. Consider adding tests for other GridTrackSize cases like auto, maxContent, and minContent to ensure comprehensive coverage of the tracks feature. For example: GridAutoColumns(.tracks([.auto, .maxContent, .minContent])).
| case .fraction(let value): | ||
| return value.rawValue | ||
| case .tracks(let values): | ||
| return values.map(\.rawValue).joined(separator: " ") |
There was a problem hiding this comment.
The implementation does not handle the edge case of an empty array. When an empty array is passed to the tracks case, it will produce an empty string as the CSS value, which may not be valid CSS. Consider adding validation or handling this edge case explicitly, either by returning a default value like "none" or by documenting that the array should not be empty.
| return values.map(\.rawValue).joined(separator: " ") | |
| return values.isEmpty ? "none" : values.map(\.rawValue).joined(separator: " ") |
| case .fraction(let value): | ||
| return value.rawValue | ||
| case .tracks(let values): | ||
| return values.map(\.rawValue).joined(separator: " ") |
There was a problem hiding this comment.
The implementation does not handle the edge case of an empty array. When an empty array is passed to the tracks case, it will produce an empty string as the CSS value, which may not be valid CSS. Consider adding validation or handling this edge case explicitly, either by returning a default value like "none" or by documenting that the array should not be empty.
| return values.map(\.rawValue).joined(separator: " ") | |
| return values.isEmpty | |
| ? GridTemplateColumns.Value.none.rawValue | |
| : values.map(\.rawValue).joined(separator: " ") |
| case .fraction(let value): | ||
| return value.rawValue | ||
| case .tracks(let values): | ||
| return values.map(\.rawValue).joined(separator: " ") |
There was a problem hiding this comment.
The implementation does not handle the edge case of an empty array. When an empty array is passed to the tracks case, it will produce an empty string as the CSS value, which may not be valid CSS. Consider adding validation or handling this edge case explicitly, either by returning a default value like "auto" or by documenting that the array should not be empty.
| return values.map(\.rawValue).joined(separator: " ") | |
| return values.isEmpty ? "auto" : values.map(\.rawValue).joined(separator: " ") |
| case .fraction(let value): | ||
| return value.rawValue | ||
| case .tracks(let values): | ||
| return values.map(\.rawValue).joined(separator: " ") |
There was a problem hiding this comment.
The implementation does not handle the edge case of an empty array. When an empty array is passed to the tracks case, it will produce an empty string as the CSS value, which may not be valid CSS. Consider adding validation or handling this edge case explicitly, either by returning a default value like "auto" or by documenting that the array should not be empty.
| return values.map(\.rawValue).joined(separator: " ") | |
| return values.isEmpty | |
| ? "auto" | |
| : values.map(\.rawValue).joined(separator: " ") |
| #expect( | ||
| renderer.renderProperty(tracks) == "grid-template-rows: 120px 2fr" | ||
| ) |
There was a problem hiding this comment.
The test only covers length and fraction track sizes. Consider adding tests for other GridTrackSize cases like auto, maxContent, and minContent to ensure comprehensive coverage of the tracks feature. For example: GridTemplateRows(.tracks([.auto, .maxContent, .minContent])).
| #expect( | ||
| renderer.renderProperty(tracks) | ||
| == "grid-template-columns: 520px 1fr" | ||
| ) |
There was a problem hiding this comment.
The test only covers length and fraction track sizes. Consider adding tests for other GridTrackSize cases like auto, maxContent, and minContent to ensure comprehensive coverage of the tracks feature. For example: GridTemplateColumns(.tracks([.auto, .maxContent, .minContent])).
No description provided.