FIX Make change tracking work again for inline editable blocks #1319
Conversation
GuySartorelli
force-pushed
the
pulls/5.3/change-tracking
branch
9 times, most recently
from
06bb1f2 to
be6eaf6
Compare
GuySartorelli
force-pushed
the
pulls/5.3/change-tracking
branch
5 times, most recently
from
1acb32b to
9125cac
Compare
| @@ -435,6 +436,7 @@ const Element = (props) => { | |||
| broken={type.broken} | |||
| onFormSchemaSubmitResponse={handleFormSchemaSubmitResponse} | |||
| onFormInit={() => handleFormInit(activeTab)} | |||
| formDirty={formDirty} | |||
There was a problem hiding this comment.
I assume this was originally removed with the intention of using the new context in the Content component - but that's a class component so it can't use hooks. Using the prop that already exists is easier and more suitable as a patch compared with completely refactoring that component to a functional one.
| @@ -8,12 +8,52 @@ Feature: Files can be saved in and removed from elemental blocks | |||
| And I add an extension "SilverStripe\FrameworkTest\Elemental\Extension\FileElementalExtension" to the "DNADesign\Elemental\Models\ElementContent" class | |||
| And I go to "/dev/build?flush" | |||
| And a "image" "file1.jpg" | |||
| And a "image" "file2.jpg" | |||
There was a problem hiding this comment.
If we add a file, save, remove the file and try to add the same file back, since it's the same file the form isn't dirty so we can't save.
The dirty state didn't change before, but now that it does we need to test this.
Anything with a file2 is for testing that scenario.
| // Note that formDirty from redux can be set to undefined after failed validation | ||
| // which is confusing as the block still has unsaved changes, hence why we create | ||
| // this state variable to track this instead | ||
| // props.formDirty is either undefined (when pristine) or an object (when dirty) | ||
| const formDirty = typeof props.formDirty !== 'undefined'; | ||
| if (formDirty && !hasUnsavedChanges) { | ||
| setHasUnsavedChanges(true); | ||
| } |
There was a problem hiding this comment.
That's not how formDirty works now - it's just a boolean same as it used to be in 5.2.
| dispatchAddFormChanged() { | ||
| // Ensures the form identifier is in unsavedForms in the global redux state | ||
| // This is used to derive the formDirty prop in mapStateToProps | ||
| dispatch(addFormChanged(`element.${elementName}`)); | ||
| }, | ||
| dispatchRemoveFormChanged() { | ||
| // Removes the form identifier from unsavedForms in the global redux store | ||
| // Opposite of beheaviour of dispatchAddFormChanged() | ||
| dispatch(removeFormChanged(`element.${elementName}`)); | ||
| }, |
There was a problem hiding this comment.
Just rely on the existing formdirty data from redux.
| @@ -24,6 +25,7 @@ Feature: Blocks are validated when inline saving individual blocks | |||
| And I press the "Insert" button | |||
| And I press the "View actions" button | |||
| And I click on the ".element-editor__actions-save" element | |||
| And I dismiss all toasts | |||
There was a problem hiding this comment.
Sometimes I was getting the "saved block" toast from this save when checking in the test scenarios.
| # Specifically swapping back to the original value - we don't actually save the block so not block save toast | ||
| And I fill in "My title" for "Title" for block 1 | ||
| And I wait for 1 second | ||
| And I press the "Save" button |
There was a problem hiding this comment.
Because change tracking works now, we can't just leave the unsaved changes hanging there.
| const [doDispatchAddFormChanged, setDoDispatchAddFormChanged] = useState(false); | ||
| const [hasUnsavedChanges, setHasUnsavedChanges] = useState(false); |
There was a problem hiding this comment.
No need to separately keep track of hasUnsavedChanges because we can just rely on formDirty instead.
No need to use doDispatchAddFormChanged because redux form keeps track of that for us automatically.
| const formDirty = state.unsavedForms.find((unsaved) => unsaved.name === `element.${elementName}`); | ||
|
|
||
| const formName = loadElementFormStateName(ownProps.element.id); | ||
| const formDirty = isDirty(`element.${formName}`, getFormState)(state); |
There was a problem hiding this comment.
This is just swapping us back to how this was in CMS 5.2. This was changed in 5.3 but it's unclear why.
Dirty state tracking worked in 5.2 so I've just reverted this back to how it was then.
| // Scenario we've just clicked "save" or "submit" on the form | ||
| if (this.state.saveAllElements && !prevState.saveAllElements) { | ||
| // Reset the validation state of all blocks. | ||
| // This mirrors handleBeforeSubmitForm() for individual blocks. | ||
| this.resetState(prevState, false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Needed because otherwise we retain the old "invalid" state. As noted in the code comment, resetting the validation state is already done when saving a block with the ... menu, so makes sense to reset it when submitting via the page save button as well.
| const saveElement = this.state.saveAllElements | ||
| && this.state.hasUnsavedChangesBlockIDs[element.id] | ||
| && this.state.validBlockIDs[element.id] === null; | ||
| const saveElement = this.state.saveAllElements && this.state.hasUnsavedChangesBlockIDs[element.id]; |
There was a problem hiding this comment.
We should attempt to save the block regardless of the validation state it was in prior to trying to save the block.
GuySartorelli
force-pushed
the
pulls/5.3/change-tracking
branch
2 times, most recently
from
939dadd to
322f459
Compare
GuySartorelli
force-pushed
the
pulls/5.3/change-tracking
branch
from
322f459 to
4d377fd
Compare
This PR effectively reverts dirty form detection for elemental back to how it was in 5.2 which was the last time it worked.
Change tracking is weirdly intertwined with a few other functionalities so other changes were also needed.
Behat tests have been made more robust to help prevent this sort of regression happening again.
Issues