FlowFuse / flowfuse

Build bespoke, flexible, and resilient manufacturing low-code applications with FlowFuse and Node-RED
https://flowfuse.com
Other
269 stars 63 forks source link

Dialogs Phase 1: dialogs should have their state handled by their parent components #4028

Open cstns opened 3 months ago

cstns commented 3 months ago

Epic

4000

Description

We need to change the way we use dialogs across the front end of the application.

We're currently using the HTMLDialogElement show method which causes concurrency issues when rendering. It is a best practice when using vue to use only vue when manipulating DOM elements.

A side effect of this is that all dialogs are present at all times in pages where they are used (but hidden with display:none) and:

General anti pattern Calling child component methods from the parent is similar to calling parent/root component methods from the child component in that it tightly couples the components together and is considered an anti-pattern. The recommended way is for two way parent-child is props passed down to child components by the parent and events emitted by child components and caught by parent components.

One example I can give, which was my own doing (I now realize), is for the blueprint tiles. Each blueprint tile has the AssetDetailDialog inside.

This implies that, with our current configuration, we have a hidden AssetDetailDialog present for each blueprint tile present on the page.

At the moment there are no real implications on this other than the fact that we have multiple hidden modals. But if we were passing props, or the dialog was using computed store props or state keys and was reacting via watchers/computed props, it would unnecessarily add computational overhead to page renders.

What I've done in https://github.com/FlowFuse/flowfuse/pull/3994/files#diff-0d94818a781a7387946662af02b3ed79ce7d1999841488af4d4985121cb3565b addresses this issue by conditionally rendering the modal in the parent. It's a small change, with the implication that the modal's state is determined externally from the dialog which somewhat breaks encapsulation but it resolves the aforementioned side effects because the dialog is not mounted until it is decided by the parent.

Which customers would this be available to

Everyone - CE/Starter/Team/Enterprise

Acceptance Criteria

check BlueprintSelectorDialog.vue for reference

Phase two should should implement a single global dialog which is present in the front end, it's state being linked to a ux store which can swap out components. It should be globally accessible and have the possibility to be interacted with across all components via the uxStore

Have you provided an initial effort estimate for this issue?

I have provided an initial effort estimate

Steve-Mcl commented 3 months ago

Phase two should should implement a single global dialog which is present in the front end

What if nested dialogs are required?

cstns commented 3 months ago

For more complex situations, using multi-part or staged modals and forms is a more effective approach.

cstns commented 3 months ago

Opening multiple dialogs at once can be confusing for users, make the interface more challenging to navigate, and disrupt tasks, which might lead to frustration and slow things down. It’s generally better to avoid using multiple dialogs because:

to name a few

joepavitt commented 3 months ago

We're currently using the HTMLDialogElement show method

Not sure I follow what you mean here?

joepavitt commented 3 months ago

By having the parent manage state, we now need to have an open/close function and dialogIsOpen state on pretty much every page (i.e. those that use dialogs). Currently, we focus that functionality within the Dialog component itself, and permit parents to call is using .show(), etc. that feels considerably cleaner than the proposal here

cstns commented 3 months ago

We're currently using the HTMLDialogElement show method

Not sure I follow what you mean here?

We're calling this.$refs.<whatever-modal>.show() from the parent which is not ok because vue is not handling the dom state in this case and that we're calling methods on child components from their parents.

By having the parent manage state, we now need to have an open/close function and dialogIsOpen state on pretty much every page (i.e. those that use dialogs). Currently, we focus that functionality within the Dialog component itself, and permit parents to call is using .show(), etc. that feels considerably cleaner than the proposal here

It is not a pretty solution for the first iteration, I agree. One way to solve it would be to extract modal interactions into a common mixin to handle them.

The end result should be a much cleaner solution, by having only one global modal in the App.vue which reacts to vuex state changes.

Consider the following example:

// store mockup
const uxStore = {
    state: {
        dialog: {
            isVisible: false,
            title: '',
            callback: null,
            component: null,
            ...
        }
    },
    mutations: {
        toggleDialogVisibility (state, { isVisible }) {
            state.dialog.isVisible = isVisible
        },
        resetDialog (state) {
            state.dialog = {
                isVisible: false,
                title: '',
                callback: null,
                component: null
            }
        },
       ...
    },
    actions: {
        openDialog ({ commit }, { component }) {
            commit('setDialogComponent', { component })
                .then(() => commit('toggleDialogVisibility', { isVisible: true }))
                .catch()
        },
        closeDialog ({ commit }) {
            commit('resetDialog')
        },
       ...
    }
}

// Dialog mockup
<dialog-component v-if="uxStore.dialog.isVisible" >
    <component :is="uxStore.dialog.component"/>
</dialog-component>

This setup would preserve the ability to have a single centralized dialog with all functionality encapsulated in the uxStore which is callable from everywhere.

The way it would work is that every 'dialog' component that you would pass in would have a mounted() method which would set it's individual title/callback/buttons/whatever and a beforeUnmount() which would reset the modal to it's initial state when closing.

By using this approach, we preserve the component lifecycle, eliminating the need for v-if=this.mounted and setting this.mounted in the mounted method. This reduces the coupling between parent and child components, avoiding scenarios where parents call child methods. Vue manages the modal DOM interactions, resulting in a loosely coupled and highly configurable solution.