themesberg / flowbite-svelte

Official Svelte components built for Flowbite and Tailwind CSS
https://flowbite-svelte.com
MIT License
1.94k stars 232 forks source link

Button id prop not passed to tag when disabled #1336

Open ncvc opened 1 month ago

ncvc commented 1 month ago

Describe the bug

The fix to this issue (commit here) introduced a bug - the id prop is no longer passed to the button tag when the Button is disabled because $$restProps is no longer being passed to svelte:element when the Button is disabled. For us, this means that we can't target a button by id with a tooltip when that button is disabled.

Reproduction

<script>
  import { Button } from 'flowbite-svelte';
</script>

<Button id="enabledButton">ID present</Button>
<Button disabled id="disabledButton">ID not present</Button>

Flowbite version and System Info

System:
    OS: macOS 14.4.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 65.88 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.12.2 - ~/.nvm/versions/node/v20.12.2/bin/node
    npm: 10.5.0 - ~/.nvm/versions/node/v20.12.2/bin/npm
  Browsers:
    Chrome: 124.0.6367.91
    Safari: 17.4.1
  npmPackages:
    @sveltejs/kit: ^2.5.7 => 2.5.7
    flowbite-svelte: ^0.46.1 => 0.46.1
    svelte: ^4.2.15 => 4.2.15
    vite: ^5.2.10 => 5.2.10
shinokada commented 1 month ago

Is adding id prop the solution?

ncvc commented 1 month ago

That would fix my specific issue, but I think users will expect all the props to be passed into the svelte:element tag when the button is disabled, since that's the behavior when it's not disabled.

Why did restProps need to be removed here? https://github.com/themesberg/flowbite-svelte/commit/11d5308785db59c70b58ad19a9819a5bbaacbc3e#diff-bb40d1bd5fefb1ab555586115b3092620c9b98dc5ddf6f33115899c19f4c442eR130

shinokada commented 1 month ago

By adding ...$$restProps, you are allowing on:click event that trigger the event even if you have disabled attribute.

ncvc commented 1 month ago

Gotcha - what if we do something like this? https://github.com/themesberg/flowbite-svelte/pull/1339

Just opened the PR to show the change, I haven't tested it since I don't have the local dev environment for this setup. Feel free to take over / edit that PR or close it and open your own