Refactor inline saving and publishing

[emteknetnz]

Follow up to #1150

The linked PR was merged with some suboptimal code due to it being open for a very long time. There are some comment in the peer review about possible ways to fix some of the suboptimal code. This card aims to fix that code.

The core reason for the suboptimal was there were existing GraphQL requests being made using a HOC pattern which meant the requests needed to be made from nested buttons, rather than centrally on Element.js. However the version of Apollo that we're now on (v3) uses hooks, and Element.js is still a class component.

Acceptance criteria

• Refactor Element.js to be a functional component.
• Refactor the save and publish GraphQL calls so they do not use the HOC pattern and instead are called within Element.js, probably via the useMutation() hook in apollo
• Refactor out the following, though only if what replaces it is less convoluted:
  • Triggering actions on state change
  • Use of setTimeout in Element.js
  • Moving context to its own file ... or remove the need for context.
  • Passing down callbacks in context (note that context is required to pass props to the buttons because they're added via js injector in registerTransforms.js)
  • Use of window.ss.apolloClient.queryManager.reFetchObservableQueries to refetch elementalArea's

New issues created

• Unable to inline save related data #1170
• Child LinkField without a link will show an error after inline save silverstripe-linkfield#276
• Block does not open when saving or publishing a blocks page with block validation errors  #1177

PRs

• ENH Refactor Element to function component #1171
• ENH Refactor Element to use mutation hook #1174
• ENH Refactor Element #1175 << refactor without graphql update
• ENH Refactor Element and graphql hoc #1176

Once the above PR is merged, assign back to Steve to create a follow up PR to do the rest of the refactoring

[GuySartorelli]

First PR merged, assigning back to Steve for next steps

[emteknetnz]

Regarding this AC:

• Passing down callbacks in context (note that context is required to pass props to the buttons because they're added via js injector in registerTransforms.js)

I don't think this is a good AC, I don't see anything wrong with passing a callback in context as it's just a variable. Context is intended a way to pass down props to nested components bypassing intermediate components so you don't need to pass through props. We pass callbacks as props all the time.

Reacts own docs include a example of putting a function into context - https://react.dev/reference/react/useContext#optimizing-re-renders-when-passing-objects-and-functions (note the use of useCallback() is labelled as a performance optimisation and isn't required)

Also we simply need to use context to pass props to SaveAction and PublishAction as they added via the injector use registerTransforms.js meaning there's no way to pass props to them the regular way

[emteknetnz]

Regarding this AC:

• Triggering actions on state change

There was an idea floated to refactor this to use promises instead

I'm not sure quite what the syntax for this looks like, I do need to plead a level of ignorance here as I find the the promise syntax fairly confusing, particularly when used inside a react component that is continuously re-rendering based on hook state changes.

I think the crux of the issue may be that we still need to keep the const [formHasRendered, setFormHasRendered] = useState(false); hook as formHasRendered is used to tell components to render the form even though that elements inline edit from hasn't been popped open yet.

I experimented with the following code in Element.js

We want to get to this point

const handleSaveButtonClick = () => {
  formHasRenderedPromise
    .then(() => {
      submitForm();
    }
}

And I used this higher up:

const formHasRenderedPromise = (resolve, reject) => {
  if (formHasRendered) {
    console.log('i resolve');
    resolve();
  }  else {
    console.log('i reject');
    reject();
  }
};

However this gets called on every render as there will be a bunch of console.log statements, as react components keep on rerendering themselves based on state changes. I guess I could try wrapping this in a useEffect() hook with [formHasRendered] as the second param, though at this point it doesn't seem like we're getting away from triggering things based on state change, we're just adding promises for a slightly better syntax in someplaces at the cost of even more convolution. "Everything is done using state hooks" seems a lot clearer to me than "Mostly we use state hooks, though there's some promises in there are well".

Unless someone can clearly explain how we can refactor the use of state (which is obviously very natural to react), to promises AND it actually reduces complexity/convolution, then I'm very inclined to just keep things as they are.

Note that we're no longer passing as many variables to the SaveAction and PublishAction via ElementContext so there has been a reduction in complexity

[GuySartorelli]

PR merged