ENH Refactor Element and graphql hoc #1176

emteknetnz · 2024-04-23T06:07:05Z

Notes:

I haven't implemented the AC "(refactor) Triggering actions on state change" - I've made a comment about it Refactor inline saving and publishing #1164 (comment) - I don't intend to implement it though if someone is able to demonstrate a non-convoluted way to implement an alternative I'm happy to refactor
I haven't implemented the 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've made a comment about it Refactor inline saving and publishing #1164 (comment). I do not intend to implement this AC
I haven't implmented the AC "Use of setTimeout in Element.js" as things don't work when setTimeout is removed, presumably due to timing issues. I have made it to the original timeout happens inside a .then() block so it always happens after the HTTP request has returned.
I haven't implemented the AC "Moving context to its own file ... or remove the need for context." as I was asked to revert it in this comment in peer review
I've removed some jest tests on SaveAction-test.js and PublishAction-test.js as there is simply now less logic and props passed via context in SaveAction and PublishAction. I choose not to add in equivalent tests in Element-test.js adding in "child Actions" would require doing stuff with injector to replicate what registerTransforms.js does now, and also because these functionality is already covered by behat tests in edit-block-element.feature (save button) and publish-block-element.feature (publish button)
I've commented out a failing behat test and raised a follow card to address Block does not open when saving or publishing a blocks page with block validation errors #1177. It's a very weird and convoluted issue which I spent a great deal of time on though was ultimately unable to resolve

GuySartorelli

Haven't actually tested it yet - will do that after these questions have been addressed in case they result in substantial changes.

client/src/components/ElementActions/PublishAction.js

GuySartorelli · 2024-04-28T23:12:21Z

client/src/components/ElementEditor/Element.js

+  useEffect(() => {
+    if (formHasRendered && doPublishElement) {
+      publishBlock({ variables: { blockId: props.element.id } })
+        .then(() => handleAfterPublish(false))
+        .catch(() => handleAfterPublish(true));
+    }
+  }, [formHasRendered, doPublishElement]);


I notice the handlePublishBlock from publishBlockMutation isn't being used here - should that be removed?

Note that handlePublishBlock has a refetchQueries that doesn't seem to be represented here.

I've just removed the unused code from publishBlockMutation(), so that it's now only the gql mutation.

handleAfterPublish() calls refetchElementalArea() which is the equivalent of what was happening with the refetchQueries in publishBlockMutation() before

GuySartorelli · 2024-04-28T23:13:20Z

client/src/components/ElementEditor/Element.js

+    // Ensure that formDirty becomes falsey after publishing
+    // We need to call at a later render rather than straight away or redux-form may override this
+    // and set the form state to dirty under certain conditions
+    // setTimeout is a hackish way to do this, though I'm not sure how else we can do this
+    // The core issue is that redux-form will detect changes when a form is hydrated for the first
+    // time under certain conditions, specifically during a behat test when trying to publish a closed
+    // block when presumably the apollo cache is empty (or something like that). This happens late and
+    // there are no hooks/callbacks available after this happens the input onchange handlers are fired
+    Promise.all(refetchElementalArea())
+      .then(() => {
+        setTimeout(() => props.dispatchRemoveFormChanged(), 250);
+      });


This is the kind of thing the refactor is intended to avoid... it feels like we're just trading one bad code smell for another here. Do you have a sense for whether the refactor has overall improved code quality? If it has then I guess I can live with this....

This is still an improvement we're always going to call props.dispatchRemoveFormChanged() after the graphql XHR in refetchElementalArea(), which previously wasn't guaranteed.

client/src/components/ElementEditor/ElementContext.js

tests/Behat/features/individual-block-validation.feature

client/src/components/ElementEditor/Element.js

GuySartorelli · 2024-04-29T22:03:29Z

Seems to work fine in the browser as-is so we're just dealing with code quality at this point. If the refetch queries change I've suggested above doesn't remove the need for the timeout, it may not be worth doing.

GuySartorelli

Looks okay. I still don't like that timeout but it sounds like we have to accept some code smells in order to remove others.

emteknetnz mentioned this pull request Apr 23, 2024

Refactor inline saving and publishing #1164

Closed

8 tasks

emteknetnz force-pushed the pulls/5/refactor-graphql3 branch 3 times, most recently from baeebb4 to 2804da8 Compare April 24, 2024 01:35

emteknetnz mentioned this pull request Apr 24, 2024

Block does not open when saving or publishing a blocks page with block validation errors #1177

Closed

3 tasks

emteknetnz force-pushed the pulls/5/refactor-graphql3 branch from 2804da8 to 1d0d954 Compare April 24, 2024 02:06

emteknetnz marked this pull request as ready for review April 24, 2024 02:07

GuySartorelli requested changes Apr 28, 2024

View reviewed changes

emteknetnz force-pushed the pulls/5/refactor-graphql3 branch from 1d0d954 to a8fc6f0 Compare April 29, 2024 00:01

GuySartorelli requested changes Apr 29, 2024

View reviewed changes

client/src/components/ElementEditor/Element.js Show resolved Hide resolved

emteknetnz force-pushed the pulls/5/refactor-graphql3 branch from a8fc6f0 to 834d692 Compare April 30, 2024 00:36

ENH Refactor Element and graphql hoc

344bc54

emteknetnz force-pushed the pulls/5/refactor-graphql3 branch from 834d692 to 344bc54 Compare April 30, 2024 00:43

GuySartorelli approved these changes Apr 30, 2024

View reviewed changes

GuySartorelli merged commit 5d9393a into silverstripe:5 Apr 30, 2024
13 checks passed

GuySartorelli deleted the pulls/5/refactor-graphql3 branch April 30, 2024 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Refactor Element and graphql hoc #1176

ENH Refactor Element and graphql hoc #1176

emteknetnz commented Apr 23, 2024 •

edited

Loading

GuySartorelli left a comment •

edited

Loading

GuySartorelli Apr 28, 2024

GuySartorelli Apr 28, 2024

emteknetnz Apr 28, 2024

GuySartorelli Apr 28, 2024

emteknetnz Apr 28, 2024

GuySartorelli commented Apr 29, 2024 •

edited

Loading

GuySartorelli left a comment

ENH Refactor Element and graphql hoc #1176

ENH Refactor Element and graphql hoc #1176

Conversation

emteknetnz commented Apr 23, 2024 • edited Loading

GuySartorelli left a comment • edited Loading

Choose a reason for hiding this comment

GuySartorelli Apr 28, 2024

Choose a reason for hiding this comment

GuySartorelli Apr 28, 2024

Choose a reason for hiding this comment

emteknetnz Apr 28, 2024

Choose a reason for hiding this comment

GuySartorelli Apr 28, 2024

Choose a reason for hiding this comment

emteknetnz Apr 28, 2024

Choose a reason for hiding this comment

GuySartorelli commented Apr 29, 2024 • edited Loading

GuySartorelli left a comment

Choose a reason for hiding this comment

emteknetnz commented Apr 23, 2024 •

edited

Loading

GuySartorelli left a comment •

edited

Loading

GuySartorelli commented Apr 29, 2024 •

edited

Loading