themesberg / flowbite-svelte

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

Components relying on setContext prevents components to be used naturally #872

Closed CapitaineToinon closed 1 year ago

CapitaineToinon commented 1 year ago

Describe the bug

I'm having issues that is caused by a button, itself inside a button group, to cause a modal to open. Because I colocate the code of the modal with the button itself, the modal and everything inside it is part of the button group. This causes some elements, such as in my case, an input, to change it styles based on the context provided by the button group.

So this line: https://github.com/themesberg/flowbite-svelte/blob/6db6b47f4dd62d3dc0a34186998bba655ae92f0e/src/lib/buttongroups/ButtonGroup.svelte#L9

Causes my input here to not be rounded: https://github.com/themesberg/flowbite-svelte/blob/6db6b47f4dd62d3dc0a34186998bba655ae92f0e/src/lib/forms/Input.svelte#L46

https://github.com/themesberg/flowbite-svelte/blob/6db6b47f4dd62d3dc0a34186998bba655ae92f0e/src/lib/forms/Input.svelte#L74-L76

This forces me to move the modal outside of the component, which in my opinion is a flaw of the library. I don't think components should be dependant from each others in such a way. I also think my use case is sound.

Good modal:

image

Broken same modal, just used elsewhere in the app:

image

Reproduction

https://stackblitz.com/edit/sveltejs-kit-template-default-dwiytk

Flowbite version and System Info

System:
    OS: macOS 13.4
    CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
    Memory: 25.73 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 16.19.0 - ~/.volta/tools/image/node/16.19.0/bin/node
    Yarn: 1.22.19 - ~/.volta/tools/image/yarn/1.22.19/bin/yarn
    npm: 8.19.3 - ~/.volta/tools/image/node/16.19.0/bin/npm
    Watchman: 2023.06.12.00 - /usr/local/bin/watchman
  Browsers:
    Chrome: 114.0.5735.133
    Edge: 114.0.1823.55
    Safari: 16.5
  npmPackages:
    @sveltejs/kit: ^1.5.0 => 1.16.3
    flowbite-svelte: ^0.38.2 => 0.38.2
    svelte: ^3.54.0 => 3.59.1
    vite: ^4.3.0 => 4.3.5
stackblitz[bot] commented 1 year ago

Fix this issue in StackBlitz Codeflow Start a new pull request in StackBlitz Codeflow.

CapitaineToinon commented 1 year ago

My current work around is to create a wrapper component that unset the context. But I'm not sure if that covers all the cases with this wrapper implementation.

<script lang="ts">
    import { Modal } from 'flowbite-svelte'
    import { setContext } from 'svelte'

    setContext('group', false)
</script>

<Modal {...$$props}>
    <slot />
</Modal>

In my opinion, if you wanna rely on context to set styles on components, every components that don't expect a specific context should just unset it for itself and its children. So for example, if button group sets group to true, then every component that isn't expect to be affected by the group styles should just unset the group for its children.

jjagielka commented 1 year ago

I'm sorry but I disagree with your approach. I don't think that encapsulating modal and the triggering button in the same component is a good idea. Components like 'modal', 'dropdown', 'tooltip' are design to decouple them from the triggering element as trigger can be very often made by multiple elements: click on the dedicated button, select from the menu or reaction to the window event. Controlling of the modal visibility should happen via variable.

<script>
  let open = false;
</script>
<Navbar>
  <NavUl>
   <NavLi on:click={()=> open=true}>Form modal</NavLi>
  </NavUl>
</Navbar>

<ButtonGroup>
  <Button on:click={()=> open=true}>Form modal</Button>
</ButtonGroup>
...
<Modal {open} />
shinokada commented 1 year ago

I close the issue for now. Please feel free to open an issue. Thank you for your contribution.