nextui-org / nextui

🚀 Beautiful, fast and modern React UI library.
https://nextui.org
MIT License
22.13k stars 1.56k forks source link

AccordionItem does not forward ref #3498

Open xylish7 opened 4 months ago

xylish7 commented 4 months ago

NextUI Version

@nextui-org/accordion@2.0.34

Describe the bug

Accordion item won't accept a ref prop.

image

Your Example Website or App

No response

Steps to Reproduce the Bug or Issue

  1. Adding ref prop to AccordionItem will result in ref not being passed to the AccordionItem since it does not support it.

Expected behavior

AccordionItem should accept the ref prop.

Screenshots or Videos

No response

Operating System Version

Windows

Browser

Chrome

linear[bot] commented 4 months ago

ENG-1156 AccordionItem does not forward ref

Tejas-Sands commented 4 months ago

can you give more context , as i am not getting this error Screenshot 2024-07-18 195842

xylish7 commented 4 months ago

Are you able to forward ref to AccordionItems? Will try to recreate the issue in a sandbox 👍

xylish7 commented 4 months ago

image

Here is a stack blitz: https://stackblitz.com/edit/stackblitz-starters-yxjqtp?file=app%2Fpage.tsx

Not sure why it fails to render the accordion (rendering a button works), but I don't care about what it renders. The issue is that AccordionItems is not taking a ref parameter.

AnthonyPaulO commented 4 months ago

This is an issue with the underlying aria component library, as their SelectItem component does not expose ref for some reason, unlike the ListboxItem which does.

xylish7 commented 4 months ago

I see. Was it reported by someone from nextui team to the aria team?

xylish7 commented 3 months ago

@AnthonyPaulO any news on this? I've seen that the AccordionItem is using a button, so not sue what SelectItem has to do with this issue.

AnthonyPaulO commented 3 months ago

@AnthonyPaulO any news on this? I've seen that the AccordionItem is using a button, so not sue what SelectItem has to do with this issue.

AccordionItem itself is a SelectItem behind the scenes, which is an Adobe Aria library component, and it does not expose a ref property. I'm not really a maintainer here so I don't have any relationship with the Aria team, but I guess I could post it as an issue on their site. I'll do that.

xylish7 commented 3 months ago

Thanks, as you have more insights into how it works! 🙏🏻

AnthonyPaulO commented 3 months ago

Thanks, as you have more insights into how it works! 🙏🏻

I created the issue here: https://github.com/adobe/react-spectrum/issues/6949

AnthonyPaulO commented 2 months ago

Okay never mind, I must have confused this with another issue so I closed the adobe react issue I created.

After a lot of digging and debugging, the real cause of the ref not working is that it's being wiped out in the useTreeState call in use-accordion.ts. Apparently the Accordion's children are cloned and passed into useTreeState and become a managed collection. This collection is then iterated over to create the individual AccordionItem elements, but the ref of each child in the collection had been stripped out during the useTreeState call so it never gets passed on.

This also occurs in ListBox via the useListState call, so those children can't have refs either, and a quick check shows it's not even implemented though it would be a quick thing to do. I presume all other components with children that use React Stately useXXXState methods suffer from the same ref-stripping issues.

snowystinger commented 2 months ago

I don't think useTreeState is at fault here, we use that hook for our Menu and I can see our ref is maintained https://codesandbox.io/p/sandbox/s6rs79

One of these is likely missing passing the ref along:

https://github.com/nextui-org/nextui/blob/5fd001c241a9324c246fdadfe23a332cc051bc88/packages/components/accordion/src/use-accordion.ts#L133 this one doesn't pass along the ref (might be ok in react 19, can't recall)

https://github.com/nextui-org/nextui/blob/5fd001c241a9324c246fdadfe23a332cc051bc88/packages/components/accordion/src/accordion.tsx#L38

Feel free to have a look through our source for more ideas.

xylish7 commented 1 month ago

@AnthonyPaulO any news on this one?