themesberg / flowbite

Open-source UI component library and front-end development framework based on Tailwind CSS
https://flowbite.com
MIT License
7.92k stars 746 forks source link

Multiple initModals and it increases backdrops #454

Closed timothymarois closed 1 year ago

timothymarois commented 1 year ago

Describe the bug If you do initModals multiple times, it will increase the number of backdrops per, though modals are designed to only init once, the backdrops do not.

To Reproduce Steps to reproduce the behavior:

  1. create multiple modals
  2. init multiple initModals (per modal)

Expected behavior It should only init 1 backdrop for all modals you init

zoltanszogyenyi commented 1 year ago

Hey @timothymarois,

Thanks for the report - that actually does make sense. If you want you can open a PR for this as it might take at least a couple of week until I do another run on issues for v1.7.0. The file where the backdrop is being added is located here:

https://github.com/themesberg/flowbite/tree/main/src/components/modal

Cheers, Zoltan

timothymarois commented 1 year ago

@zoltanszogyenyi I took a look into it and after further testing, this needs a refactor. Its much bigger issue. The instances are not being stored in memory, you use the instances on init and then they are wiped out. Which means, every click event, is actually running a a click X amount of inits you do, performance-wise not a good approach here. This setup is only good if you run a single init on your page. but in SPA, this is not a suitable option.

I dug deeper and tried to remove click event and re-add etc to avoid duplication, but it wasnt that, it was the fact that you cant actually see what instances are visible, they are all independent and theres no source of truth for the instances themselves. In my example, playing around with the Drawers, but same could be said about modals.

If you run a SPA that dynamically load new components in, these init functions are rendered completely useless. Im sorry, but they cant be used in their current form. I was really hoping this would be a great solution to my UI, it would just take a lot of work for me to refactor this class and given its not my package, I bet that large of a refactor wouldnt go over so well.

Quick notes:

timothymarois commented 1 year ago

And to add, the goal of the init should be instantiated every single time a component is mounted, otherwise the init is 100% useless in a true SPA environment. Imagine running the inits on every modal component loaded in the background, every button you apply more inits, you'd essentially have dozens of potential extra clicks clogging up the bandwidth.

The ideal solution here:

timothymarois commented 1 year ago

Final conclusion for tonight, I copied over the drawer code into my project directly, modified your getDrawerInstance and used my own state, once applied the state to that instance array, it works perfectly. (with one modification on show, using if (!this.isVisible()) { to stop it in its tracks.) This works when it actually has access to the instances. But again, to add state management, you'd need to do a bit of a refactor to make this happen. In the current form, I cant advice the inits being used in any SPA.

timothymarois commented 1 year ago

My solution for now on Nuxt3 using composables, and not relying on the inits for drawers/modals. This works really well. And creating the vue components that log the instances based on the Drawer/Modal classes.

interface Instance {
    id: string;
    object: object;
}

export const useInstances = (): Ref<Instance[]> => {
    return useState('instances', () => []);
}

export const addInstance = (id: string, object: object) => {
    const instance = getInstance(id);
    const instances = useInstances()
    if (!instance) {
        instances.value.push({id, object})
    }
}

export const getInstance = (id: string) => {
    const instances = useInstances();
    if (instances.value.some((instance) => instance.id === id)) {
        return instances.value.find((instance) => instance.id === id);
    }
}

export const getInstanceIndex = (id: string) => {
    const instances = useInstances();
    const index = instances.value.findIndex((instance) => instance.id === id);
    if (index !== -1) {
        return index;
    }
}

export const removeInstance = (id: string) => {
    const instances = useInstances();
    instances.value = instances.value.filter(instance => instance.id !== id)
}

export const hideInstance = (id: string) => {
    const instance = getInstance(id)
    if (instance) instance.object.hide()
}

export const showInstance = (id: string) => {
    const instance = getInstance(id)
    if (instance) instance.object.show()
}

export const toggleInstance = (id: string) => {
    const instance = getInstance(id)
    if (instance) instance.object.toggle()
}
zoltanszogyenyi commented 1 year ago

Hello @timothymarois,

Thanks for all the valuable feedback - I believe there is room for improvement in the "instances" department on Flowbite. Indeed, currently they are being lost as it is kept only on a framework JS level and can be re-created only on page load or using the init functions.

I would be more than willing to work with you on a refactoring of these init functions or at least apply a solution to the modal components where we can take it to the other ones as well.

The code that you've outlined in your latest example would be a pretty good API to offer for each instance of a component that will be created using Flowbite.

You can DM me on the Discord Server (my tag is zoltanszogyenyi) as the contributors also chat there quite a lot. I have to go to rest now but I'll check the issues tomorrow and Discord especially if you're willing to work on a solution together.

Cheers, Zoltan

archonic commented 1 year ago

I believe this might be related to an issue I'm seeing but I'm not calling initModal (directly) at all. I have a turbo-frame inside of a drawer. Upon opening the drawer, two backdrops exist. Upon closing, just one backdrop is removed. If there's no eager loaded content in the drawer, this doesn't happen.

anthonyfranc commented 1 year ago

Having the same issue as well, showing 4 backdrops

Princekrampah commented 1 year ago

Having the same issue with sidebar. Any feedback on this?

ameeralic commented 1 year ago

same issue here white using sidebar with nav component creates 4 layers of

it won't remove while clicking outside. but works fine while clicking hamburger icon.

meinto commented 1 year ago

Workaround - manage drawer manually:
https://flowbite.com/docs/components/drawer/#example

import { useRef } from "react";
import { Drawer } from "flowbite";

const PageWithDrawer = () => {
   const drawerRef = useRef<Drawer | null>(null);

   return (
      <div>
         <div onClick={() => drawerRef.current?.show()}>Button to open drawer</div>
         <div
            ref={(ref) => {
               if (!drawerRef.current) {
                  drawerRef.current = new Drawer(ref, {
                     placement: "left",
                     backdrop: true,
                  });
               }
            }}
         >Drawer</div>
      </div>
   )
}
truongduyng commented 1 year ago

With rails using turbo setup, the init functions are called even more multiple times if we load turbo frame in the page. For my case, it loads 10 backdrop for a sidebar 😅 Does anyone have a wordaround for now? 🙏

yasoob commented 1 year ago

It has been a while and this issue is still catching people off guard. I just got bit by it as well. Any update on this? :)

Oddadmix commented 1 year ago

Any updates on this please?

zoltanszogyenyi commented 1 year ago

Hey everyone!

Flowbite v2.0 fixes this issue by bringing a brand new Instances Manager.

Please read more about it here:

https://flowbite.com/docs/getting-started/javascript/ https://github.com/themesberg/flowbite/releases/tag/v2.0.0

Cheers, Zoltan