Virtual components#65
Conversation
thetarnav
left a comment
thetarnav
left a comment
There was a problem hiding this comment.
I guess since it's from sky then it's working.
| width={1620} | ||
| scroll="always" | ||
| autofocus | ||
| itemsPerRow={7} |
There was a problem hiding this comment.
isn't this just columns?
| itemsPerRow={7} | |
| columns={7} |
| import { VirtualGrid } from './components/VirtualGrid'; | ||
|
|
||
| <VirtualGrid | ||
| id="BrowseGrid" |
| - `display: flex`, `flexWrap: wrap`, `gap: 30` | ||
| - `transition: { y: 250ms ease-out }` |
There was a problem hiding this comment.
| - `display: flex`, `flexWrap: wrap`, `gap: 30` | |
| - `transition: { y: 250ms ease-out }` | |
| - `display: "flex"`, `flexWrap: "wrap"`, `gap: 30` | |
| - `transition: { y: {duration: 250, easing: 'ease-out'} }` |
| - **onEndReached** (`() => void`): Callback triggered when selection moves near the end of the list. | ||
| - **children** (`(item: Accessor<T>, index: Accessor<number>) => JSX.Element`): Function that renders each item. | ||
| - **selected** (`number`): Initial selected index. | ||
| - **scrollToIndex** (`(index: number) => void`): Imperatively scrolls and focuses the item at the given index. |
There was a problem hiding this comment.
What is scrollToIndex? its not a part of props type, nor the prop is used anywhere.
| })); | ||
| } | ||
|
|
||
| export const clamp = (value: number, min = 0, max = 100) => |
There was a problem hiding this comment.
what makes 0 - 100 a good default?
if you read somewhere clamp(x) do you immediately know that its clamping the value between 0 and 100?
I'd prefer clamp(x, 0, 100) tbh.
| onLeft={/* @once */ lngp.chainFunctions(props.onLeft, rowOnLeft)} | ||
| onRight={/* @once */ lngp.chainFunctions(props.onRight, rowOnRight)} | ||
| forwardFocus={/* once */ lngp.onGridFocus(chainedOnSelectedChanged)} |
There was a problem hiding this comment.
| onLeft={/* @once */ lngp.chainFunctions(props.onLeft, rowOnLeft)} | |
| onRight={/* @once */ lngp.chainFunctions(props.onRight, rowOnRight)} | |
| forwardFocus={/* once */ lngp.onGridFocus(chainedOnSelectedChanged)} | |
| onLeft={/* @once */ lngp.chainFunctions(props.onLeft, lngp.navigableHandleNavigation)} | |
| onRight={/* @once */ lngp.chainFunctions(props.onRight, lngp.navigableHandleNavigation)} | |
| forwardFocus={lngp.navigableForwardFocus} |
| x: { | ||
| duration: 250, | ||
| easing: 'ease-out', | ||
| }, |
There was a problem hiding this comment.
Why not use the user-defined default?
| x: { | |
| duration: 250, | |
| easing: 'ease-out', | |
| }, | |
| x: true, |
|
|
||
| const rowStyles: lng.NodeStyles = { | ||
| display: 'flex', | ||
| gap: 30, |
There was a problem hiding this comment.
Can we drop the default gap width of 30?
Every user will have a different design system and different gaps for different components.
There is no point in trying to guess the default value for everyone.
Default gap of 0 is the most expected, as this is what you get with display: "flex", you change that el to Row and suddenly you have some random gap.
| const onUp: lngp.KeyHandler = function () { | ||
| const perRow = itemsPerRow(); | ||
| const selected = this.selected || 0; | ||
| if (selected < perRow) return false; | ||
|
|
||
| const newIndex = utils.clamp(selected - perRow, 0, items().length - 1); | ||
| const lastIdx = selected; | ||
| this.selected = newIndex; | ||
| const active = this.children[this.selected]; | ||
| if (active instanceof lng.ElementNode) { | ||
| active.setFocus(); | ||
| chainedOnSelectedChanged.call(this as lngp.NavigableElement, this.selected, this as lngp.NavigableElement, active, lastIdx); | ||
| } | ||
| }; | ||
|
|
||
| const onDown: lngp.KeyHandler = function () { | ||
| const perRow = itemsPerRow(); | ||
| const selected = this.selected || 0; | ||
|
|
||
| const newIndex = utils.clamp(selected + perRow, 0, items().length - 1); | ||
| if (newIndex !== selected) { | ||
| const lastIdx = selected; | ||
| this.selected = newIndex; | ||
| const active = this.children[this.selected]; | ||
| if (active instanceof lng.ElementNode) { | ||
| active.setFocus(); | ||
| chainedOnSelectedChanged.call(this as lngp.NavigableElement, this.selected, this as lngp.NavigableElement, active, lastIdx); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
These could be merged together.
Or the spatialHandleNavigation could be used.
|
I think the spatial stuff is only in RC3 - so will change that around when we get to it. |
2 participants
I've tested VirtualGrid - going to push up the demo changes. Take a look at let me know what you think.