New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unrequested refresh of configuration forms/Reset configuration forms on cancel. #2399

Merged
merged 7 commits into from Jun 23, 2016

Conversation

Projects
None yet
2 participants
@dennisoelkers
Member

dennisoelkers commented Jun 22, 2016

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.

@dennisoelkers dennisoelkers added the bug label Jun 22, 2016

@dennisoelkers dennisoelkers modified the milestones: 2.0.4, 2.1.0 Jun 22, 2016

if (typeof this.props.onModalClose === 'function') {
this.props.onModalClose();
_onModalCancel() {
if (typeof this.props.onCancel === 'function') {

This comment has been minimized.

@edmundoa

edmundoa Jun 22, 2016

Member

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.

@edmundoa

edmundoa Jun 22, 2016

Member

Another point on this topic: what would you expect to happen when you are editing the modal and press "Escape"?

This comment has been minimized.

@dennisoelkers

dennisoelkers Jun 23, 2016

Member

onModalClose is still called. We need to differentiate those two cases, because resetting the form is not required after submitting it, while it is necessary when cancelling it.

I did not consider the Escape-case (and clicking the x-button or simply the backdrop), will take care of it.

This comment has been minimized.

@edmundoa

edmundoa Jun 23, 2016

Member

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.

@dennisoelkers

dennisoelkers Jun 23, 2016

Member

I think anything else than resetting the form when explicitly dismissing it does not make sense, and I have yet to see an app where explicitly closing a form does not reset it.

In this scenario onModalClose is called through the explicit call of BootstrapModalWrapper.close() from BootstrapModalForm. _onModalCancel().

This comment has been minimized.

@edmundoa

edmundoa Jun 23, 2016

Member

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.

@dennisoelkers

dennisoelkers Jun 23, 2016

Member

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.

@edmundoa

edmundoa Jun 23, 2016

Member

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.

typeName: React.PropTypes.string,
values: React.PropTypes.object,
},

This comment has been minimized.

@edmundoa

edmundoa Jun 22, 2016

Member

❤️

@@ -53,6 +65,7 @@ BootstrapModalWrapper.propTypes = {
]).isRequired,
onOpen: PropTypes.func,
onClose: PropTypes.func,
onCancel: PropTypes.func,

This comment has been minimized.

@edmundoa

edmundoa Jun 23, 2016

Member

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.

@dennisoelkers

dennisoelkers Jun 23, 2016

Member

That is correct. I will rename it to onHide just as the Modal component itself names it.

<BootstrapModalWrapper ref="modal"
onOpen={this.props.onModalOpen}
onClose={this.props.onModalClose}
onCancel={this._onModalCancel}>

This comment has been minimized.

@edmundoa

edmundoa Jun 23, 2016

Member

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.

@dennisoelkers

dennisoelkers Jun 23, 2016

Member

You are correct, I fixed that.

@edmundoa edmundoa self-assigned this Jun 23, 2016

@edmundoa

This comment has been minimized.

Member

edmundoa commented Jun 23, 2016

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.

Other than that, looks good to me 👍

@edmundoa edmundoa merged commit 78f694c into master Jun 23, 2016

4 checks passed

ci-server-integration Jenkins build graylog2-server-integration-pr 1012 has succeeded
Details
ci-web-linter Jenkins build graylog-pr-linter-check 498 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@edmundoa edmundoa deleted the fix-configuration-forms branch Jun 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment