WPEX 1690 - refactor post carousel to remove JQuery #2235
Conversation
snovosel
requested review from
AnthonyLedesma,
EvanHerman,
jonathanbardo,
jrtashjian and
olafleur-godaddy
as code owners
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
|
Download coblocks.zip: https://43405-128991767-gh.circle-artifacts.com/0/tmp/artifacts/coblocks-2235.zip |
Performance Test Results:
|
| } ); | ||
|
|
||
| helpers.checkForBlockErrors( 'coblocks/post-carousel' ); | ||
| // helpers.checkForBlockErrors( 'coblocks/post-carousel' ); |
This reverts commit d2df8d4.
| setTimeout( () => { | ||
| const postCarouselContainer = document.querySelector( `.wp-block-coblocks-post-carousel-external-container-${ carouselUuid }` ); | ||
|
|
||
| // remove the swiper classes so that the external feed does not display within carousel in editor | ||
| const swiperWrapper = postCarouselContainer.querySelector( '.swiper-wrapper' ); | ||
|
|
||
| if ( swiperWrapper ) { | ||
| swiperWrapper.classList.remove( 'swiper-wrapper' ); | ||
| swiperWrapper.classList.add( 'swiper-wrapper-editor' ); | ||
|
|
||
| const swiperBackButton = postCarouselContainer.querySelector( '#wp-coblocks-post-carousel-swiper-prev' ); | ||
| const swiperNextButton = postCarouselContainer.querySelector( '#wp-coblocks-post-carousel-swiper-next' ); | ||
|
|
||
| swiperBackButton.style.display = 'none'; | ||
| swiperNextButton.style.display = 'none'; | ||
| } | ||
| }, 1000 ); |
There was a problem hiding this comment.
Why does this need a timeout? Could it instead be an async function where we await for the dom elemnts?
There was a problem hiding this comment.
this is triggering specifically the SSR portion of this block, which renders the output. In this case, because the classnames in the editor conflict with the classnames on the front end script, we need to in some way remove the class so that the front end script (which will run regardless) does not invoke the TinySwiper package. I'm not sure what you mean by async function.
There was a problem hiding this comment.
Ok so this is meant to prevent the Swiper script from initializing in the editor. I see also the src/js/coblocks-post-carousel.js script has a timeout. Is that related to this timeout for the same reason by chance? Or do you mind explaining the timeout there? I am curious if the timeout is needed because there might not be any elements available (like a race condition) also. I saw something similar with the Counter front-end script where the script could load before the related dom elements.
There was a problem hiding this comment.
basically the issue was that when the script was run initially the elements that it is looking for were not rendered and not able to be queryable in order to do the necessary changes. I believe that this script runs before any other rendering and so waiting the timeout allowed the markup to mount to the DOM so that we can make any necessary changes
| export default compose( [ | ||
| withSelect( ( select, { clientId } ) => { | ||
| const blockAttributes = select( 'core/block-editor' ).getBlockAttributes( clientId ); | ||
|
|
||
| return { | ||
| attributes: blockAttributes, | ||
| }; | ||
| } ), | ||
| ] )( PostItem ); |
There was a problem hiding this comment.
I am confused why this is necessary at all. We are making a store call here when this component has access to the execution context where the attributes exist. On line 183 or 159 of src/blocks/post-carousel/edit.js. Why not simply pass in the existing attributes? You can modify the PostItems like this:
<PostItem key={ i } post={ post } { ...{ ...props } } />This change will give you full access to the existing props preventing the need for an extra compose and data-store call.
There was a problem hiding this comment.
Because the PostItem component is rendered dynamically via the Swiper component (through render-props https://reactjs.org/docs/render-props.html ) , the individual PostItem components are not triggered to re-render via the parent PostEdit. Instead, they are connected to the store and will re-render following any dynamic prop update to which they are listening. In the GalleryCarousel a similar pattern is used, though using the Context API.
This way, the Swiper component itself is not re-rendered following any attribute updates, and only the PostItem components are re-rendered specifically in order to minimize any unnecessary re-renders.
There was a problem hiding this comment.
Minimization of excess renders sounds like a positive for sure. I suspect there is something I am missing still. I guess I am not clear about how if these attributes are already available in the parent and should not change dynamically why we would expect excess renders. When the attribute changes on the parent, the children would re-render as expected because the passed attribute and related useEffect function are watching.
There was a problem hiding this comment.
I feel like I am about to learn something lol.
There was a problem hiding this comment.
if you look at the below function within the Swiper component,
const renderList = useMemo( () => {
return list.map( ( item, index ) => (
<div className={ `swiper-slide` } id={ index } key={ index }>
{ children( {
index,
item,
} ) }
</div>
) );
}, [ isDraggable ] );
The Swiper component is invoked within the PostEdit and uses a single child which is a function :
{ ( { index: i, item: post } ) => {
return (
<PostItem clientId={ clientId } key={ i } post={ post } setAttributes={ setAttributes } />
);
} }
The rendering order would be as follows --> PostEdit ---> Swiper --> PostItem. However, within the Swiper component function renderList, the function itself is only recalculated following the update of the isDraggable variable since the function is memoized. Therefore, the children function would only be updated if that variable is changed. This separates the concern of the Swiper and the child component that it is rendering and allows us to abstractly use the Swiper component without any information of what it is rendering, and when it should update. It would only update when Swiper specific props are changed. That being said, the children component will have the first render of the attributes if we pass it down as props, though when the attribute changes , the swiper component will not update itself with the updated attributes and so instead we can use redux to listen to any store updates , but within the component that needs that information instead of drilling props down through each level.
There was a problem hiding this comment.
I appreciate this explanation of why. This makes sense then to keep.
src/components/swiper/index.js
Outdated
| <button className={ `nav-button__prev ${ navigation === false && 'no-navigation' }` } id={ `${ uuid }-prev` }> | ||
| <button className={ navigationClass ? `${ navigationClass }__prev` : `nav-button__prev ${ navigation === false && 'no-navigation' }` } id={ `${ uuid }-prev` }> | ||
| <svg className="icon" style={ { transform: 'rotate(180deg)' } } /> | ||
| </button> | ||
| <button className={ `nav-button__next ${ navigation === false && 'no-navigation' }` } id={ `${ uuid }-next` }> | ||
| <button className={ navigationClass ? `${ navigationClass }__next` : `nav-button__next ${ navigation === false && 'no-navigation' }` } id={ `${ uuid }-next` }> |
There was a problem hiding this comment.
This is a little difficult to read now that we are nesting a tertiary as well here. How do you feel about moving this to use the classnames package? For the sake of posterity, I think it would help with understanding the conditional.
/**
* External dependencies
*/
import classnames from 'classnames';
const buttonClasses = classnames( {
[ `${ navigationClass }__next` ]: navigationClass,
'nav-button__next': ! navigationClass,
'no-navigation': navigation === false
} );There was a problem hiding this comment.
updated this in the newest commit
@AnthonyLedesma I just updated the space between for the slides on the post carousel. that should handle the style overlap |
|
I pulled down those latest style changes and rebuilt them. I am still on TT1 here. I am still getting some alignment strangeness on other slides just not that first one now. Here is what I am seeing.
|
|
The front-end seems to have some strangeness here too still. Still see an overlap in this Gif. Also, notice the time to load the Swiper carousel. I wonder if this is because there are 3 on one page. I reload twice in this image to show the time to load.
|
|
@AnthonyLedesma I am still not seeing the issue you have in your video but it does not look like you have the updated changes. There would be spacing between the slides per the newest commit Screen.Recording.2022-02-02.at.4.04.46.PM.movregarding the alignment, where in the block settings is this located? I don't see that option |
Screen.Recording.2022-02-02.at.4.08.24.PM.mov@AnthonyLedesma screen recording of the front end including the newest commit |
That I show in the following video. It's in the block toolbar.
|
|
@AnthonyLedesma updated the styles in front end and editor to account for the full width and wide width |
Description
This PR removes jQuery from the post carousel block in an effort to remove jQuery from the coblocks project
Screenshots
Types of changes
refactor
How has this been tested?
visually, cypress
Checklist: