feat(Accordion): add multiple prop and close others by default

[oritwoen]

Resolves #362
Resolves #356

This PR adds to the Accordion component an additional mode of operation where you can customize the closing of panels when a specific list item is opened or global closing of panels whenever 1 list item is open.

[nuxt-studio]

✅ Live Preview ready!

Name	Edit	Preview	Latest Commit
ui	Edit on Studio ↗︎	View Live Preview	5485b1c

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
ui	🔄 Building (Inspect)		Jun 28, 2023 8:10pm

[vercel]

@oritwoen is attempting to deploy a commit to the Nuxt Labs Team on Vercel.

A member of the Team first needs to authorize it.

[Haythamasalama]

@oritwoen, nice additions! ❤️

[benjamincanac]

Nice one! I'd rather have a multiple prop (instead of closeOthers) that is false by default. When multiple is false, only one item at a time can be open.

[oritwoen]

@Haythamasalama I corrected the types in the code.

@benjamincanac Right multiple fits better. From what you wrote I understood that the default multiple is supposed to be false.

This creates a small breaking change for users who already used Accordion in their somewhere. But actually Accordion I generally associate with one element open at a time and that fits the default mode. Unless I misunderstood your message?

I have updated the code and documentation based on the above issues.

For the items objects, I left the closeOthers variable as it was because there, in my opinion, it fits appropriately - it closes others if the multiple mode is set to true.

There was also the issue of transition for "one at a time mode" which I wanted to do based on the max-height animation but apparently you resolved that issue very efficiently here: f3c6f83

[atinux]

This creates a small breaking change for users who already used Accordion in their somewhere. But actually Accordion I generally associate with one element open at a time and that fits the default mode. Unless I misunderstood your message?

So far it is in Edge so we are find to change the behaviour before it becomes stable.

[benjamincanac]

@oritwoen It's ok to let multiple to false as indeed this component hasn't been published yet.

There is still a timing issue with the transition when multiple is false, we can see a slight shift between the open of the new item and the close of the other above or below.

Why not to let closeOthers in the items as it might be useful to some!

Also, this behaviour doesn't work when overriding the button in the default slot. How can we achieve that without passing the closeAll function and asking users to bind it to their button?

[Haythamasalama]

@benjamincanac
@oritwoen

I'm wondering if we can rename multiple to multiple-open ?
I think it would be clearer to indicate their association with opening and closing.

[Haythamasalama]

@oritwoen
I am unable to push to this PR to assist you in achieving it. However, I would like to contribute by adding something

1- To solve the issue with the closeAll function and slot:

In the Accordion component, pass :close-all="closeAll" to the slot:

 <HDisclosureButton :ref="() => updateClosesRefs(close, index)" as="template" :disabled="item.disabled">
    <slot :item="item" :index="index" :open="open" :close="close" :close-all="closeAll">
// ..

In the AccordionExampleDefaultSlot, add closeAll and @click="closeAll(index)":

  <UAccordion :items="items" :ui="{ wrapper: 'flex flex-col w-full' }">
    <template #default="{ item, index, open , closeAll }">
      <UButton //.... @click="closeAll(index)">

I have tested it, and it works correctly.

2- It is unnecessary to add as HTMLElement in (el as HTMLElement).

  function onLeave (el: HTMLElement, done) {
  //  (el as HTMLElement).addEventListener('transitionend', done, { once: true })
   
    el.addEventListener('transitionend', done, { once: true })
  }

The el parameter is already declared as type HTMLElement, so the type assertion is not required.

[benjamincanac]

@Haythamasalama That was my point, I don't like the idea of asking users to manually assign the function themself when overriding the default slot. I'm looking for another way to achieve this!

[benjamincanac]

I've updated the behaviour with a child component that emits when the open changes. This way, there is no click function to bind on the buttons. Let me know what you think 😊

[Haythamasalama]

I've updated the behaviour with a child component that emits when the open changes. This way, there is no click function to bind on the buttons. Let me know what you think 😊

This way it's very perfect ❤️

But I still noticed a slight shift between button items, however, not with slot-item it's work well.

[benjamincanac]

Indeed, I've noticed it too. This is because of gap-y-2 on the wrapper. I've tried with space-y-2 or adding margin directly to the buttons but it does the same. I'll create an issue to keep track of this and merge this pr.

[Haythamasalama]

Indeed, I've noticed it too. This is because of gap-y-2 on the wrapper. I've tried with space-y-2 or adding margin directly to the buttons but it does the same. I'll create an issue to keep track of this and merge this pr.

I tried to remove gap-y-2 and adding mb-1 to button class and i think it's work well ?

[oritwoen]

Sorry guys but due to internet problems I couldn't work on the pull request the last few days. I had some comments/corrections but I see that the merge is already done. When I have a stable network, I'll just do additional PR in terms of performance or drafts with the thoughts that came to my mind.

Thanks :)