Dropdown does not work properly when using ToolbarButton in each loop

[maku]

Describe the bug

When using the combination of Dropdown with ToolbarButton in an each loop I get the wrong result.

e.g. in the example (https://gitlab.com/maku_at/flowbite-test) I have a component "Places" which uses an each loop to display a list of items. For every item the "Place" component is used to display a card and a Menu (with ToolbarButton and DropdownItem). The Place component has a parameter called "place" for the data object (export let place...)

But when the Dropdown is clicked via the ToolbarButton it is using sometimes the wrong data instance. E.g. for "Place 1" the data instance of "Place 2" is used...

Reproduction

https://gitlab.com/maku_at/flowbite-test

System Info

System:
    OS: Windows 10 10.0.22621
    CPU: (12) x64 Intel(R) Core(TM) i7-8700K CPU @ 3.70GHz
    Memory: 12.67 GB / 31.93 GB
  Binaries:
    Node: 16.15.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.19 - C:\Program Files\nodejs\yarn.CMD
    npm: 8.16.0 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Chrome: 104.0.5112.102
    Edge: Spartan (44.22621.457.0), Chromium (104.0.1293.70), ChromiumDev (106.0.1363.0)
    Internet Explorer: 11.0.22621.1

[shinokada]

@jjagielka

[jjagielka]

In your code:

<ToolbarButton class="dots-menu text-gray-900 bg-white dark:text-white dark:bg-gray-800" />
<Dropdown class="w-44" triggeredBy=".dots-menu">
    <DropdownItem on:click={editPlace}>Edit {place.name}</DropdownItem>
</Dropdown>

you set the dropdown trigger to all elements on the page with the dots-menu class (and you create many of them). So by clicking on toolbar button you in fact open all existing dropdowns, one over the other.

To correct that you need your dropdown to be triggred only by the specific button. The easiest way, is to use fact that if there's no triggeredBy property set, dropdown will attach itself to the previous sibling.

Long story short, remove the triggeredBy property from your code.

[maku]

@jjagielka thank you for the hint. In my simple example app it works when I remove "triggeredBy", but in real live I am using flowbite-svelte in a "tauri" app on windows (https://tauri.app) and here it does not work... So I tried to put an "id" attribute on the ToolbarButton and a "triggeredBy" to that Dropdown id but no success. (this has to be caused by the "Webview" used by tauri)

Would be an alternative approach to bind the Dropdown and set open to true when the button is clicked?

[jjagielka]

So I tried to put an "id" attribute on the ToolbarButton and a "triggeredBy" to that Dropdown id

There was a bug with ToolbarButton id settings. It's fixed only now. Please update flowbite-svelte.

Note, that with id setting approach you'll need a unique id for each ToolbarButton so some kind of id generator would be needed. The below is tested to work.

<script lang="ts" context="module">
    let _id: number = 0;
    const getId = () => ++_id;
</script>

<script lang="ts">
...
    export let place: IPlace;

    export function editPlace() {
        console.log('editPlace', place);
    }

    const id = 'tlb-' + getId();
</script>

<Card class="m-4">
...
    <ToolbarButton {id} class="dots-menu text-gray-900 bg-white dark:text-white dark:bg-gray-800" />
    <Dropdown>
        <Dropdown class="w-44" triggeredBy={'#' + id}>
            <DropdownItem on:click={editPlace}>Edit {place.name}</DropdownItem>
        </Dropdown>
    </Dropdown>
...
</Card>

[maku]

@jjagielka Great thank you for the fast fix. I am really impressed by your work for the project 👍

[jjagielka]

On the other hand it's not a good practice to have a per component dropdown while they can be shared.

I'd suggest moving Dropdown to the loop level, triggered by the class, and set current value via store.

store.js

import { writable } from 'svelte/store';
export default writable(() => {});

Place.svelte

<script>
  import store from './store';
...
  export function editPlace() {
        console.log('editPlace', place);
  }
</script>
...
<div>
<ToolbarButton class="dots-menu text-gray-900 bg-white dark:text-white dark:bg-gray-800"
        on:click={() => store.set(editPlace)} />
<!-- no Dropdown here - moved to Places.svelte -->
</div>

Places.svelte

<script lang="ts">
    import store from './store';
...
    function handle(e) {
        $store();
    }
</script>

<div class="p-8">
    <h3>Places</h3>

    <div class="p-8">
        {#each places as place}
            <Place {place} />
        {/each}
    </div>
</div>

<Dropdown class="w-44" triggeredBy=".dots-menu">
    <DropdownItem on:click={handle}>Edit</DropdownItem>
</Dropdown>

With that you have only one instance of Dropdown shared by all ToolbarButton. And you don't have to play if id attributes.

Note! Avoid using the store directly in on:click handle as that fool out svelte. <Dropdown on:click={() => $store()}>Edit</Dropdown>

[maku]

@jjagielka Could it be that the implementation of DropdownItem is incorrect. Currently there is an if based on "href". which leads to the situation that without an href param the events are not triggered (e.g. on:click)

[maku]

On the other hand it's not a good practice to have a per component dropdown while they can be shared.

I'd suggest moving Dropdown to the loop level, triggered by the class, and set current value via store.

store.js

import { writable } from 'svelte/store';
export default writable(() => {});

Place.svelte

<script>
  import store from './store';
...
  export function editPlace() {
      console.log('editPlace', place);
  }
</script>
...
<div>
<ToolbarButton class="dots-menu text-gray-900 bg-white dark:text-white dark:bg-gray-800"
      on:click={() => store.set(editPlace)} />
<!-- no Dropdown here - moved to Places.svelte -->
</div>

Places.svelte

<script lang="ts">
  import store from './store';
...
  function handle(e) {
      $store();
  }
</script>

<div class="p-8">
  <h3>Places</h3>

  <div class="p-8">
      {#each places as place}
          <Place {place} />
      {/each}
  </div>
</div>

<Dropdown class="w-44" triggeredBy=".dots-menu">
  <DropdownItem on:click={handle}>Edit</DropdownItem>
</Dropdown>

With that you have only one instance of Dropdown shared by all ToolbarButton. And you don't have to play if id attributes.

Note! Avoid using the store directly in on:click handle as that fool out svelte. ~<Dropdown on:click={() => $store()}>Edit</Dropdown>~

Thank you for the suggestion. But from my point of view the Dropdown should be placed where it belongs (Place.svelte). Of course you could it do it the described way. But this overcomplicates the handling to maybe get a smaller app. But thank you for the suggestion... :-)

[jjagielka]

@jjagielka Could it be that the implementation of DropdownItem is incorrect. Currently there is an if based on "href". which leads to the situation that without an href param the events are not triggered (e.g. on:click)

Please read the docs: https://flowbite-svelte.com/components/dropdown#Events

If you take an example of something like Dropdown_with_checkbox your suggestion above will lead to different sources of click event coming from the same element: one from DropdownItem and one from the checkbox inside.

[maku]

@jjagielka ok I get it -> but in the impl of DropdownItem.svelte in the {#if href}branch

    this={href ? 'a' : 'div'}

has no sense, right?

[jjagielka]

Yes, I've spotted that. Will be removed in the next update.

[maku]

Thank you, only a suggestion: maybe it would be easier to grasp when it would be checked if anything is passed in for the slot (via $$slots) and in the other case do the same as in the "if href" branch...

[jjagielka]

Can you please put pseudo code for what you propose? Even in the if href branch there is a <slot/>, so what would you check?

[maku]

Not sure if the source is correct but something like:

<script lang="ts">
   ...
    let hasSlot = $$slots && $$slots['default'];
</script>

{#if !hasSlot}
    <li>
        <svelte:element
            this={href ? 'a' : 'div'}
            {...$$restProps}
                        {href}
            class={classNames(liClass, colors[color] ?? colors.default, $$props.class)}
            on:click
            on:change
            on:keydown
            on:keyup
            on:focus
            on:blur
            on:mouseenter
            on:mouseleave>
            <slot />
        </svelte:element>
    </li>
{:else}
    <li class={classNames(liClass, colors[color] ?? colors.default, $$props.class)}>
        <slot />
    </li>
{/if}

[jjagielka]

That's the problem that you have <slot/> in both branches. So which one to detect?

[maku]

o damn I overlooked that... Forget all what I said, sorry... That would only work when you cancel the slot in the first branch and provide a property like name/description ...

[maku]

But what about to remove the if completley. Only using 1 branch and "passing the event trought" -> coming from other UI libs, I would expect an "on:click" on DropdownItem (but of course it is only "learning" to do it the correct way)

[jjagielka]

If we are to remove the if I'd rather remove the first branch and leave simple li placeholder with padding and coloring:

<li class={classNames(liClass, colors[color] ?? colors.default, $$props.class)}>
    <slot />
</li>

then the usage would be:

<DropdownItem><OtherComponentOrElement>My label and/or icon</OtherComponentOrElement></DropdownItem>

In practice however the most common use case is to insert <a/> with href and not even using on:click:

<DropdownItem><a href="/">My label and/or icon</a></DropdownItem>

and therefore the first branch.

[jjagielka]

The way of thinking is:

<Dropdown>
  <DropdownItem href="/">Simple link</DropdownItem>
  <DropdownItem href="/"><Icon /> Simple link with icon</DropdownItem>
  <DropdownItem>
     <Label><input on:change={}/>Advanced usage with full control</Label>
     <Button on:click={}>And multiple elements</Button>
  </DropdownItem>
</Dropdown>

[maku]

you've convinced me. then you should leave it as is... I just assumed there was an on:click because I looked at the source examples and didn't read the documentation at the end (the events section).

BTW, I think the examples are misleading (-> false)

e.g.

<DropdownItem on:click={() => dropdownOpen = false}>Overview (close)</DropdownItem>

It is in this form in the docu but does not work....

[jjagielka]

Well spotted. Marked as a bug and needs correction. Thanks.