This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Fix JS-rendered blocks inside Empty Cart #2904
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Aljullu
added
type: bug
|
Size Change: +2.91 kB (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
Aljullu
force-pushed
the
fix/2836-js-rendered-blocks-inside-js-rendered-blocks
branch
from
342d810
to
fdd4758
Compare
Aljullu
force-pushed
the
fix/2836-js-rendered-blocks-inside-js-rendered-blocks
branch
2 times, most recently
from
1bbfedf
to
71b4d4c
Compare
Aljullu
force-pushed
the
fix/2836-js-rendered-blocks-inside-js-rendered-blocks
branch
2 times, most recently
from
28b72d2
to
932739f
Compare
Aljullu
force-pushed
the
fix/2836-js-rendered-blocks-inside-js-rendered-blocks
branch
from
932739f
to
c1410d6
Compare
Aljullu
requested review from
a team and
senadir
and removed request for
a team
Aljullu
commented
Aug 4, 2020
| // the empty cart. In those cases, we don't want to trigger the render function of | ||
| // inner components on load. Instead, the wrapper block can trigger the event | ||
| // `wc-blocks_render_blocks_frontend` to render its inner blocks. | ||
| const selectorsToSkipOnLoad = [ '.wp-block-woocommerce-cart' ]; |
There was a problem hiding this comment.
For now, I'm hard-coding the Cart class name here, but if we see this is needed in other blocks, we could try to dynamically fill this array when blocks are rendered with the Data Registry.
senadir
approved these changes
Aug 4, 2020
| { bubbles = false, cancelable = false, element } | ||
| ) => { | ||
| if ( ! element ) { | ||
| element = document.body; |
There was a problem hiding this comment.
why not have this in the destructuring declaration?
There was a problem hiding this comment.
Ah, I don't know why. I thought it was breaking e2e tests but now it seems to work fine. Changed it in 04ae943.
There was a problem hiding this comment.
Had to revert this. Taking a closer inspection what happened was:
document.body.querySelector( ... )returnednullif it didn't find the element, soelementwas null.- when destructuring a nullish object property, the default value doesn't seem to be applied, so
elementwas not getting assigned todocument.body, instead it kept thenullvalue.
TIL
const wizard = { name: 'Harry', middleName: null, lastName: 'Potter'};
const { middleName = 'James' } = wizard;
console.log( middleName ); // null
There was a problem hiding this comment.
👍 if middleName was undefined however, then James would be the value. null is considered a value which is why the default doesn't get assigned.
Comment on lines
82
to
104
| /** | ||
| * Adds the event listeners necessary to render the block frontend. | ||
| * | ||
| * @param {Object} props Render props. | ||
| */ | ||
| export const renderFrontend = ( props ) => { | ||
| const wrappersToSkipOnLoad = document.body.querySelectorAll( | ||
| selectorsToSkipOnLoad.join( ',' ) | ||
| ); | ||
| // Render on page load. | ||
| renderBlockFrontend( { | ||
| ...props, | ||
| wrappersToSkip: wrappersToSkipOnLoad, | ||
| } ); | ||
| // Render wrappers inner blocks when the event `wc-blocks_render_blocks_frontend` | ||
| // is triggered. | ||
| Array.prototype.forEach.call( wrappersToSkipOnLoad, ( wrapper ) => { | ||
| wrapper.addEventListener( 'wc-blocks_render_blocks_frontend', ( e ) => { | ||
| renderBlockFrontend( { ...props, wrapper: e.target } ); | ||
| } ); | ||
| } ); | ||
| }; | ||
|
|
There was a problem hiding this comment.
While I get the higher-level logic, I'm having a hard time understanding the logic happening here, what exactly are we doing? waiting for an event to render again?
There was a problem hiding this comment.
renderFrontend does three things:
- Gets the wrapper elements that should be skipped on page load. That means that inner blocks of those wrappers should not be rendered on page load but instead when an event on the wrapper is triggered.
- Renders the block frontend for all blocks which are not inside those wrappers.
- For each of those wrappers, adds an event listener so, when the
wc-blocks_render_blocks_frontendevent is triggered, inner blocks are rendered.
In 535002c, I spent some time renaming functions, variables and re-organizing code trying to make code a bit simpler. Hopefully it's a bit easier to understand now. 🙂
Remove Suspense compatibility fix once WP 5.2 is no longe...Remove Suspense compatibility fix once WP 5.2 is no longer supported. If Suspense is not available (WP 5.2), use a noop component instead. https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/a343fc08c6d02ac9476ba6131eb8f8a3e03fb7c5/assets/js/base/utils/render-frontend.js#L46-L58🚀 This comment was generated by the automations bot based on a
|
Aljullu
force-pushed
the
fix/2836-js-rendered-blocks-inside-js-rendered-blocks
branch
from
a343fc0
to
535002c
Compare
|
Looks like there's some JS tests failing that need addressed here before merge. |
This reverts commit 04ae943.
|
It seemed to be caused by 04ae943, I reverted it and tests are passing again. |
nerrad
deleted the
fix/2836-js-rendered-blocks-inside-js-rendered-blocks
branch
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2836.
This PR refactors how
renderFrontendworks so blocks that might contain JS-rendered inner blocks can trigger awc-blocks_render_blocks_frontendJS event to render the inner blocks.Screenshots
How to test the changes in this Pull Request:
Changelog