bharbron / hexbook-semantic

An Electron-React-Redux-Semantic-UI application for automatically generating a hexcrawl campaign notebook from random tables.
5 stars 3 forks source link

Rethink the way we're handling state for editing template properties and metadata #49

Closed bharbron closed 5 years ago

bharbron commented 6 years ago

We have an interesting situation with the EditTemplateModal. In order to know whether to enable the SAVE button, allow submission of changes, etc. the modal component needs to know whether all of the input in the modal is valid, including the template properties and metadata.

However, the structure of the properties and the metadata varies depending on which template plugin is being used. So the EditTemplateModal component cannot do validation of properties and metadata itself. We have to trust that task to the specific template plugin.

This leads to a situation where the parent component can't control the value props of the child component, but needs to know their state. The parent modal needs to know whether properties and metadata are in a "valid" state, and needs to know their values for when the modal submits. But actual control of those needs to be done in the child, because only the individual template plugin knows anything about the structure of that data, what fields to show, what values are needed, whether values are "valid", etc.

Right now, I'm working around this by having the template plugins updating both the usual, asynchronous local state, and a synchronously updated copy of the state. That copy is then sent up to the parent component via an onChange handler.

This feels like a hack though, and is probably not best practice for React. Let's do some research, find out what other people have done in this case in React, and possibly rework this. It is functional right now, but buts a lot of responsibility on the writer of template plugins to make sure they're updating everything in two different copies of the state.

bharbron commented 5 years ago

Some brainstorming on this that was preventing me from sleeping last night:

Control properties and metadata from the parent EditTemplateModal component. Provide them to each template plugin's EditProperties and EditMetadata components as props. Do not keep a copy of properties and metadata in the child components local store.

Also store properties.valid and metadata.valid in the parent component, not the child component (although the child component may want to track valid for individual parts of properties and metadata if they're significantly complicated).

For onChange() in the child component, create a new properties or metadata object (e.g. const properties = {...this.props.properties, foobar: {updates to foobar}}). Validate the the object in the child component (e.g. invalid characters in input, duplicate entries, etc.). Send the properties or metadata object to the parent along with the valid state to the parent component via this.props.onChange(properties, valid). The parent component will then replace the entire local state.properties or state.metadata with the object it got from the child, as well as updating the valid value in its local state.

Errors should be managed in the child component, so that input can display pop-up error messages, etc. Only the valid value needs to be sent up to and controlled by the parent. Errors should be generated by functions that use props.properties and props.metadata as input, rather than stored in the state.

We should also do a better job of creating components around Form.Input, Form.Select, etc. that include the Ref, Popup, etc. stuff to make it easier for people to develop new template plugins.

bharbron commented 5 years ago

Actually, storing error in state might not be a bad idea. But, if we do that, pass it up to the parent as part of onChange(), have the parent store it, and pass it to the child as a props.

There is the question of how to determine if the initial properties and metadata are valid when a modal is first opened. There really shouldn't be a case where this happens -- i.e. all template plugins provide "valid" defaults, no way to ever save an "invalid" template, etc.

bharbron commented 5 years ago

This has been done. State has been lifted up to the parent TemplateEditModal