canonical / react-components

A set of components based on Vanilla Framework
https://canonical.github.io/react-components
92 stars 51 forks source link

What to do with propagation of click events in ConfirmationModal? #1032

Closed vladimir-cucu closed 5 months ago

vladimir-cucu commented 6 months ago

Currently, when we press the Cancel, Confirm or the close button in ConfirmationModal component, the click propagates further to underlying components. In Juju Dashboard we have a panel that triggers the opening of a ConfirmationModal that doesn't overlap the panel (see attached screenshot). When we press any of the aforementioned 3 buttons, a click outside the panel is triggered as well, which closes the panel. However, we want the click outside the panel to not be triggered. We could add event.nativeEvent.stopImmediatePropagation() in juju dashboard where necessary, but I was wondering if we could instead include this in the Modal component itself? We had a similar issue with propagating the click of Esc button in Modal component and we fixed it by stopping the propagation of the event.

Screenshot from 2024-01-30 13-27-43

bartaz commented 6 months ago

Sounds like something to bring up on React Guild.

On the first look it seems a valid issue, any clicks that are actually handled by the modal (on modal buttons, or on modal overlay to close it, etc), should probably NOT trigger anything else.

I wonder should we just implement it into the component? Are there any cases where we would actually like the events to propagate instead?

Current implementation seems to leave it to developer to decide - by default all propagates - if you don't want it to happen, stop it yourself. I guess the question is, is the propagation ever needed? Should modal just always "contain" its events and not propagate them further?

vladimir-cucu commented 6 months ago

Discussing these questions at the React Guild sounds like a good idea. I'll remember to add it to the notes before the meeting next Thursday.

bartaz commented 5 months ago

As discussed in React Guild - we are fine with stopping propagation inside the modal by default.

If we ever have a case where we need the propagation, we can add it as an option.

vladimir-cucu commented 5 months ago

I could take a look at fixing this issue if nobody began looking at it. Should we disable the event propagation by default and add an optional prop stopEventPropagation = true to the Modal component?

github-actions[bot] commented 5 months ago

:tada: This issue has been resolved in version 0.51.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: