fix(list): updates typescript types for children prop #7164
Conversation
| onClickItem?: | ||
| | ((event: React.MouseEvent) => void) | ||
| | ((event: { item?: ListItemType; index?: number }) => void); | ||
| onClickItem?: ( | ||
| event: React.MouseEvent & { item: ListItemType; index: number }, | ||
| ) => void; |
There was a problem hiding this comment.
Based on a read-through of the code, the correct type is a function with the intersection of the event and what it gets modified with as the single argument
| p: React.PropsWithChildren<ListExtendedProps<ListItemType>>, | ||
| p: ListExtendedProps<ListItemType>, |
There was a problem hiding this comment.
This is what was clobbering the type in such a weird way
| <List data={data} paginate={{ background: 'red', margin: 'large' }} /> | ||
| <List data={data} paginate={{ margin: 'large' }} /> |
There was a problem hiding this comment.
It looks like background isn't actually a valid paginate prop
There was a problem hiding this comment.
Good catch, I think removing the background here is fine
| if (!$element) | ||
| throw new Error('Cannot find element with id "alphaMoveDown"'); |
There was a problem hiding this comment.
This short-circuits so that fireEvent doesn't get passed null
| action={(item, index) => ( | ||
| action={(_, index) => ( |
There was a problem hiding this comment.
There was a lint error about unused arguments and the _ fixes that
| children?: ( | ||
| item: ListItemType, | ||
| index: number, | ||
| state?: { active: boolean }, | ||
| ) => any; |
There was a problem hiding this comment.
I am getting a typescript error on the active prop if I try to do something like this: (item, index, {active}) => {}.
There was a problem hiding this comment.
Ah, I misread the documentation - I thought the entire state object was optional rather than just the properties within. Fix incoming!
There was a problem hiding this comment.
Just ran into an issue - I modified the test to validate and it seems that it's actually the entire state object that is optional. Pinging on Slack for follow-up semi-synchronous conversation and will report back the decision for posterity's sake.
There was a problem hiding this comment.
Discussed offline, we are going to hold off on trying to support destructuring the active prop in the typescript definition. I think we can move forward with the types that are defined in this PR.
| <List data={data} paginate={{ background: 'red', margin: 'large' }} /> | ||
| <List data={data} paginate={{ margin: 'large' }} /> |
There was a problem hiding this comment.
Good catch, I think removing the background here is fine
What does this PR do?
This PR fixes the TypeScript types for
ListExtendedPropsto align with the documentation.Where should the reviewer start?
There isn't much to this PR. I would start with reading through #7160 and then pulling the code down to test in a development environment.
What testing has been done on this PR?
Migrated the tests to Typescript and modified the object children test
How should this be manually tested?
Use it in a development environment.
What are the relevant issues?
Closes #7160
Screenshots (if appropriate)
Do the grommet docs need to be updated?
No, although I'm not sure what the type for the third argument should be based on the docs, so I left it a vague
Record<string, any>Should this PR be mentioned in the release notes?
No
Is this change backwards compatible or is it a breaking change?
Backwards compatible unless people leveraged module augmentation or similar to get around the issue.