react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

SelectItem missing ref prop while ListboxItem implements/exposes it.

Open AnthonyPaulO opened this issue 1 year ago โ€ข 3 comments

Provide a general summary of the issue here

Unlike ListBoxItem, SelectItem doesn't implement/expose a ref prop.

๐Ÿค” Expected Behavior?

SelectItem should implement/expose a ref prop.

๐Ÿ˜ฏ Current Behavior

ref prop is missing.

๐Ÿ’ Possible Solution

Implement ref prop.

๐Ÿ”ฆ Context

No response

๐Ÿ–ฅ๏ธ Steps to Reproduce

The following is a sandbox link to a NextUI implementation that uses an AccordionItem component, which is really just an Aria SelectItem component behind the scenes. As you can see, it's missing the ref prop due to the underlying SelectItem component not implementing/exposing it.

https://stackblitz.com/edit/stackblitz-starters-yxjqtp?file=app%2Fpage.tsx

Version

React Aria 3.34.3

What browsers are you seeing the problem on?

Chrome

If other, please specify.

No response

What operating system are you using?

Windows 11

๐Ÿงข Your Company/Team

No response

๐Ÿ•ท Tracking Issue

No response

AnthonyPaulO avatar Aug 26 '24 20:08 AnthonyPaulO

Thanks for the Issue. Is this actually one of our components? https://github.com/nextui-org/nextui/blob/canary/packages/components/accordion/src/accordion-item.tsx I'm not seeing any of our components in this file.

Also, not sure what a SelectItem is, that's not one of our components. We have one in a tailwind starter, but that can be edited and changed as you need.

The stackblitz doesn't seem to be building. Can you check you saved it in a good state?

snowystinger avatar Aug 27 '24 05:08 snowystinger

was the one to raise the issue in the nextui repo (https://github.com/nextui-org/nextui/issues/3498). The stackbliz won't build because for whatever reasons it doesn't want to render the AccordionItem, but The issue is with that ref prop missing on AccordionItem (the IDE is highlighting it).

I also told @AnthonyPaulO that the AccordionItem doesn't seem to implement any SelectItem. You can follow the entire conversation here: https://github.com/nextui-org/nextui/issues/3498.

P.S. @snowystinger I really appreciate your quick responses to issues ๐Ÿ™

xylish7 avatar Aug 28 '24 15:08 xylish7

We can wait for them to explain.

In the meantime, I think you're just getting a Typescript error? is it not actually attached either?

I see the AccordionItem does take a ref https://github.com/nextui-org/nextui/blob/canary/packages/components/accordion/src/accordion-item.tsx#L13 so I'm a bit surprised by the TS error.

It looks like it's passed through to a button https://github.com/nextui-org/nextui/blob/4f8ae50cf441da5b5685b6573714a3f7c9ab3ea2/packages/components/accordion/src/use-accordion-item.ts#L158 so I'd be surprised if it wasn't attached either.

Are you on an older version of Next? it looks like they used to use a different forwardRef signature about a year ago from looking at the git blame.

snowystinger avatar Aug 28 '24 20:08 snowystinger

I thought it was only an issue with Typescript but it was not actually being forwarded. I will upgrade all dependencies to latest and come back with the result.

xylish7 avatar Aug 29 '24 07:08 xylish7

Here is a stackblizz example that shows my problem: https://stackblitz.com/edit/vitejs-vite-pfvddt?file=src%2FApp.tsx.

It seems like it's not a typescript issue, @AnthonyPaulO please confirm that it is indeed a react-aria issue and not a nextui one.

image

xylish7 avatar Aug 29 '24 14:08 xylish7

We can wait for them to explain.

In the meantime, I think you're just getting a Typescript error? is it not actually attached either?

I see the AccordionItem does take a ref https://github.com/nextui-org/nextui/blob/canary/packages/components/accordion/src/accordion-item.tsx#L13 so I'm a bit surprised by the TS error.

It looks like it's passed through to a button https://github.com/nextui-org/nextui/blob/4f8ae50cf441da5b5685b6573714a3f7c9ab3ea2/packages/components/accordion/src/use-accordion-item.ts#L158 so I'd be surprised if it wasn't attached either.

Are you on an older version of Next? it looks like they used to use a different forwardRef signature about a year ago from looking at the git blame.

Sorry I was on vacation, back now.

I have to go back and see what the exact issue was, but now that I think about it and recall a bit more I believe it was due to aria library not exposing the ref property in the typescript definition file. I'll check after I settle back in, but I think it missing the ref prop definition was the issue.

AnthonyPaulO avatar Sep 02 '24 19:09 AnthonyPaulO

Okay I don't know what I was smoking, I dug through this and must have confused this with another issue so I'm closing this bug report. The real issue is that useTreeState wipes out the children refs, so it's an issue with our usage of Adobe's React Stately and not with the components themselves. My apologies!

AnthonyPaulO avatar Sep 04 '24 21:09 AnthonyPaulO

Thanks for looking into it again and also thanks @snowystinger!

xylish7 avatar Sep 10 '24 07:09 xylish7