-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Disallow invalid number of columns for narrow and medium Grids #1628
fix: Disallow invalid number of columns for narrow and medium Grids #1628
Conversation
The CSS classes we render from these props can receive the same treatment. Although, the last time I tried, I ran into a gotcha that I can’t remember now. Please perform sufficient visual tests on different window widths with high and low values, single and triplets, etc. |
Oh, and please do the same for |
…/DES-962-grid-max-span-for-smaller-viewports
…/DES-962-grid-max-span-for-smaller-viewports
…hub.com:Amsterdam/design-system into fix/DES-962-grid-max-span-for-smaller-viewports
…/DES-962-grid-max-span-for-smaller-viewports
@@ -21,9 +26,9 @@ type BreakoutCellSpanAllProp = { | |||
|
|||
type BreakoutCellSpanAndStartProps = { | |||
/** The amount of grid columns the cell spans. */ | |||
colSpan?: 'all' | GridColumnNumber | GridColumnNumbers | |||
colSpan?: 'all' | GridNarrowColumnNumber | GridMediumColumnNumber | GridWideColumnNumber | GridColumnNumbers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has overlap: 1 | 2 | 3 | 4 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | …
We discussed Pick<>
, but that’s for object types, not literals.
This may be an option:
type Subset<T, U> = T extends U ? T : never
export type GridColumnNumber = 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12
export type GridColumnNumbers = {
narrow: Subset<GridColumnNumber, 1 | 2 | 3 | 4>
medium: Subset<GridColumnNumber, 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8>
wide: GridColumnNumber
}
Types that accept all three variations can keep using GridColumnNumber
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an alternative. The definition is more complex, but usage is simpler.
type Range<N extends number, Result extends Array<unknown> = []> = Result['length'] extends N
? Result[number]
: Range<N, [...Result, Result['length']]>
export type GridColumnNumber = Range<12>
export type GridColumnNumbers = {
narrow: Range<4>
medium: Range<8>
wide: Range<12>
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subset seems best?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I like how simple it is to use Range
even if the definition is more complex.
I did something similar in the experimental branch:
type Enumerate<N extends number, Acc extends number[] = []> = Acc['length'] extends N |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end we chose the KISS solution. :-)
…/DES-962-grid-max-span-for-smaller-viewports
…hub.com:Amsterdam/design-system into fix/DES-962-grid-max-span-for-smaller-viewports
Describe the pull request
There is a undesirable feature in
Grid
that allows setting a higher number of columns than is possible for that specific breakpoint.What
Do not allow developers to set an invalid number of columns per media breakpoint on a
Grid.Cell
in aGrid
via TypeScript.Why
It restricts the freedom of developers to write invalid code.
How
Added separate
GridNarrowColumnNumber
andGridMediumColumnNumber
types.Checklist
Before submitting your pull request, please ensure you have done the following. Check each checkmark if you have done so or if it wasn't necessary:
Additional notes