Conversation
|
Size Change: +63 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 2497ee6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4077659498
|
| <Button onClick={ onMoveUp } { ...props }> | ||
| Move Up | ||
| </Button> | ||
| ) } | ||
| </TreeGridCell> | ||
| <TreeGridCell> | ||
| { ( props ) => ( | ||
| <Button onClick={ onMoveDown } { ...props }> | ||
| Move Down | ||
| </Button> |
There was a problem hiding this comment.
Simplified this code snippet a bit to cut down on the number of lines.
| if ( ! focusablesInRow || ! focusablesInRow.length ) { | ||
| return; | ||
| } | ||
| } ) as FocusableElement[]; |
There was a problem hiding this comment.
Anything that is returned from this function can be assumed to have a focus() method.
There was a problem hiding this comment.
Why not use HTMLElement directly instead of FocusableElement?
Or at least, could we Pick< HTMLElement, 'focus' > ?
There was a problem hiding this comment.
You're right, I think I misunderstood something. Fixed in 52cbdd9
There was a problem hiding this comment.
Thank you!
I was wondering if we could push this further and see if more (if not all) of the Element types in this component can be changed to HTMLElement?
For example, I noticed that onFocusRow / onCollapseRow / onExpandRow were previously documented as accepting an HTMLElement
| if ( | ||
| hasModifierKeyPressed || | ||
| ! [ UP, DOWN, LEFT, RIGHT, HOME, END ].includes( keyCode ) | ||
| ! ( [ UP, DOWN, LEFT, RIGHT, HOME, END ] as number[] ).includes( |
There was a problem hiding this comment.
Apparently, importing these keycode constants implies that they are really consts.
// Normally, doing this in a single file is ok.
const UP = 30;
const foo = (n: number) => {
return [UP].includes(n)
}
// But when `UP` is imported (I think) it becomes equivalent to
const UP = 30 as const;
// And then this won't pass
const foo = (n: number) => {
// Argument of type 'number' is not assignable to parameter of type '30'.
return [UP].includes(n)
}🤷🤷🤷
| if ( ! treeGridElement.contains( activeElement ) ) { | ||
|
|
||
| if ( | ||
| ! activeElement || |
There was a problem hiding this comment.
This early return is just to appease TS, but actual runtime behavior should not change because if activeElement were falsey treeGridElement.contains( activeElement ) would also be falsey.
| | { | ||
| lastFocusedElement: Element | undefined; | ||
| setLastFocusedElement: React.Dispatch< | ||
| React.SetStateAction< Element | undefined > | ||
| >; | ||
| } | ||
| | undefined | ||
| >( undefined ); |
There was a problem hiding this comment.
It would be easy to set up a "safer" default for this context value (for example an empty object {}), but a unit test implies that we should actually be throwing if the context is not properly initialized. So I'm leaving this as is (just more explicitly undefined I guess).
| return children( allProps ); | ||
| } | ||
|
|
||
| if ( ! Component ) return null; |
There was a problem hiding this comment.
Just an added guard, but nevertheless a runtime change.
Interestingly, there are no actual usages of this as prop in the repo.
There was a problem hiding this comment.
Maybe there were when the as = Component prop destructuring was introduced, but since then it was removed
There was a problem hiding this comment.
Did some more investigation — this component was introduced as a separate component from TreeGrid and it seemed to have the as prop from its beginnings: https://github.com/WordPress/gutenberg/pull/18014/files#diff-b3f07d9e77626753ea74b8a24a73ef0f82bc0a9d537617976b8ad34415f13ac0
The as prop listed in the docs, in the Storybook examples, and even in the unit tests, but I don't think other components were using it at that point.
The component was later incorporated into TreeGrid as part of #22373
| children: { control: { type: null } }, | ||
| }, | ||
| parameters: { | ||
| actions: { argTypesRegex: '^on.*' }, |
There was a problem hiding this comment.
Note that this won't fire actions for onExpandRow until we add a specific story that allows it to happen. I'm not going to do that in this PR because it is somewhat non-trivial.
There was a problem hiding this comment.
What is the specific config that we need to replicate? Is it worth adding it to the onExpandRow prop docs?
There was a problem hiding this comment.
I think a good example would need to implement a simple version of a file browser where you can expand folder rows. Most of the functionality isn't baked into the component so we'd need to manage it ourselves (toggling visibility, state, styling, etc). We can write a better description for onExpandRow once we better understand what it entails.
| as?: React.ComponentType< RovingTabIndexItemPassThruProps >; | ||
| [ key: string ]: any; |
There was a problem hiding this comment.
Honestly, in terms of TypeScript I think things could be a lot simpler if we didn't allow an as prop, and didn't allow pass-through of arbitrary props. All the same things would be possible with just a children render prop and no arbitrary pass-through props. We might want to revisit this API if these loose types cause more trouble than it's worth.
There was a problem hiding this comment.
Absolutely, let's keep this in mind
| const meta: ComponentMeta< typeof TreeGrid > = { | ||
| title: 'Components (Experimental)/TreeGrid', | ||
| component: TreeGrid, | ||
| subcomponents: { TreeGridRow, TreeGridCell }, |
There was a problem hiding this comment.
I am hesitant to add TreeGridItem to the subcomponents table because:
- It wasn't even documented in the main readme.
- It seems quite low level, and I'm not sure if the few usages in the Gutenberg app are even warranted in the first place 🤔
- The docgen refuses to generate anything, probably because the types are too loose. (It needs to accept any prop, as they are passed through to arbitrary
children/as.)
There was a problem hiding this comment.
The only reason why I'd add it is to "manifest" its existence in the TreeGrid family — ie., if you use TreeGrid, you should TreeGridItem too (since it enabled the roving tab index functionality)
There was a problem hiding this comment.
if you use
TreeGrid, you shouldTreeGridItemtoo (since it enabled the roving tab index functionality)
I think the original intent was for people to use TreeGridCell (which is TreeGridItem wrapped in a <td> tag). Using TreeGridItem without the gridcell wrapper is an invitation for semantic issues. There are two instances in the Gutenberg app where a raw TreeGridItem is used, and I have a feeling these are both problematic. If I understand the treegrid spec correctly, there should not be multiple focusable elements in a gridcell.
There was a problem hiding this comment.
Makes sense. Let's keep it out of the docs, and probably prepare a follow-up to see if we can fix those TreeGridItem usages and convert them to TreeGridCell
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Great work here!
I noticed that there a few mentions in the component's docs about the fact that the component internally renders a table / td / tr — do you think we should keep those references, even if they expose the internal DOM structure of the component?
Also, we're missing a CHANGELOG entry 🗒️
| if ( ! focusablesInRow || ! focusablesInRow.length ) { | ||
| return; | ||
| } | ||
| } ) as FocusableElement[]; |
There was a problem hiding this comment.
Why not use HTMLElement directly instead of FocusableElement?
Or at least, could we Pick< HTMLElement, 'focus' > ?
| if ( | ||
| hasModifierKeyPressed || | ||
| ! [ UP, DOWN, LEFT, RIGHT, HOME, END ].includes( keyCode ) | ||
| ! ( [ UP, DOWN, LEFT, RIGHT, HOME, END ] as number[] ).includes( |
| // TODO: The original implementation simply used `ref.current` here, assuming | ||
| // that a forwarded ref would always be an object, which is not necessarily true. | ||
| // This workaround maintains the original runtime behavior in a type-safe way, | ||
| // but should be revisited. |
There was a problem hiding this comment.
Nice comment.
I guess the better way here could be to check if typeof ref === 'function' (i.e. a callback ref) and then act accordingly. Definitely a task for a follow-up, and not this PR.
| as?: React.ComponentType< RovingTabIndexItemPassThruProps >; | ||
| [ key: string ]: any; |
There was a problem hiding this comment.
Absolutely, let's keep this in mind
| children: { control: { type: null } }, | ||
| }, | ||
| parameters: { | ||
| actions: { argTypesRegex: '^on.*' }, |
There was a problem hiding this comment.
What is the specific config that we need to replicate? Is it worth adding it to the onExpandRow prop docs?
|
Another quick note — the "Move up" / "Move down" buttons in the Storybook example don't currently do anything when clicked — should we have them at least print something to console? |
Yes, I think those semantics are an essential part of the component, and thus part of the non-breaking contract. Otherwise it would just be the roving tabindex behavior.
🙈
I agree they're confusing. I swapped them out with text fields so they look less actionable. Wdyt? cd66379
|
There was a problem hiding this comment.
I think this looks better and less confusing, thank you!
Since there are a lot of open convos that are not actionable for this PR, here's a quick recap for our future selves:
Needs addressing in this PR:
ElementtoHTMLElement(convo)
To be addressed separately from this PRs: (low-priority)
- adding a specific Storybook example to highlight how to use the
onExpandRowcallback (convo) - consider removing the
asprop (convo, convo) - see if we can clean remove all usages of
TreeGridItemand replace them withTreeGridCell(convo) - Make the
refcheck inRovingTabIndexItemsupport callback refs too (convo)
DaisyOlsen
added
the
[Type] Code Quality
3 participants
Part of #35744
What?
Convert the TreeGrid component and its subcomponents to TypeScript, including stories and tests.
How?
See inline comments.
Testing Instructions
npm run storybook:devand check the Docs view for TreeGrid.