Conversation

edited

This PR fixes two issues with the configuration forms:

When they are refreshing their props (either because the form is reinitialized or because a plain re-render is triggered by a store refresh) the internal state is reinitialized with a mixture of the default values and the passed props, but not the already existing state which included changes made by the user. The outcome is that configuration forms which are part of a DOM subtree which is refreshed by a store refresh, periodically snap back into their default values.

The configuration form's internal state is set back to the default values when the form is closed. This prevents that when e.g. a new input form is opened and some changes are made, closing and reopening it will still show the old values.

Both of these are also proper fixes for #1870 and #1929, which were fixed by preventing their periodical refresh at that time.

This comment has been minimized.

This change will break some of the other components using BootstrapModalForm, as before they relied on the onModalClose callback being called when the modal was cancelled or closed, and now it won't be called all the time.

Anyway, why do we need to differentiate between both situations? The way I see it, closing the modal and clicking cancel is the same. Most of the time I also feel that the software I use behaves in that way too (closing a modal is usually triggering the same action as cancelling). That doesn't apply to "reset" buttons, but usually you can't find those in modals.

This comment has been minimized.

I still don't see why we need to differentiate both cases. I think submitting the form also needs to reset the inputs in some cases (after creating an entity, for instance). I also think that some people will be mad because they pressed cancel instead of the "x" or escape and their inputs are gone, specially in forms using textareas like an alert callback email template or a sidecar snippet. At least I would be mad if I needed to re-enter the input for that reason. At the moment we are fairly inconsistent with that (as with many other things), but I see this as a step backwards in usability.

Regarding onModalClose not being called, I meant on external components using BootstrapModalForm. See this example for instance: The MesageProcessorConfig expects the onModalClose callback to be called on close or cancel, but now it will only be called on close, changing the previous behaviour of that component.

This comment has been minimized.

There's one example that came into my mind as I read your comment: editing emails. Few apps nowadays will delete the text you wrote even if you close the window or the application. In most situations in our app, the user just need to fill in a name and description, and it's not a big issue, but in others it may need to add passwords, or even big chunks of text.

This change is specially tricky, because we are doing different things when closing or cancelling the submit, which is in my opinion a technicality, and will lead to error and frustration.

I'll also add a quote from Jef Raskin, author of The Humane Interface:

The system should treat all user input as sacred.

Regarding the the onModalCancel, I missed that, so ✅

This comment has been minimized.

The MTU case implies that there is some drafting mechanism, which we do not have (and is probably overkill for our use case). Those applications which are much closer to our use case are all following the semantics implemented by this PR, which means that when the user is cancelling (the action of doing this is btw a user input as well) the "draft" is discarded.

Another argument for changing the behavior we have today: when editing an existing input, making changes in one of the fields, cancelling the form and editing the entity again shows the changes that were just done and cancelled. This is imo unexpected for a majority of the users and leads to an inconsistency between the persisted state of the entity, the state that is shown in the overview and the state as shown in the edit form.

This comment has been minimized.

I don't really see it the same way, and also don't consider that a user making the modal go away, is implying that their changes are gone. We are imprecise by nature, and will find the easiest way to make that go away, even if you just want to look at some detail or copy something from somewhere else.

Either way, I don't think we can convince each other on this, and I don't want to discuss this forever. As long as the changes work consistently, we can go with them.

This comment has been minimized.

I don't see why we need this prop here, BootstrapModalWrapper has nothing to do with cancelling, or forms. It is simply a wrapper to make handling modals a bit easier, or at least that is its motivation.

This comment has been minimized.

After 6ce4be2, things are starting to get confusing again. Shouldn't we rename the original onModalClose to onCancel, and just keep a single callback?

The thing is, if I didn't miss anything, closing the modal and cancelling is doing the same thing right now: call the onCancel callback if available, and then call close on the BootstrapModalWrapper, that will eventually call the onModalClose prop. Well, when the modal is closed with escape, clicking outside, or pressing the "x", we call onModalClose twice.

This comment has been minimized.

These changes will require a consolidation on how we treat data when modals are closed, cancelled. Looking through the web interface, this is another point of inconsistency on our side. Will create a follow up ticket for that.

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.