Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PLANET-6874: Fix the Gallery block #932

Merged
merged 5 commits into from Mar 15, 2023

Conversation

GP-Dan-Tovbein
Copy link
Contributor

@GP-Dan-Tovbein GP-Dan-Tovbein commented Aug 25, 2022

Ref: https://jira.greenpeace.org/browse/PLANET-6874

Description

Major changes

  • hydrate the component because it was using a deprecated method when saved it. Now it's a fully SSR Component.
  • Fix Gallery styles, not only the slider.
  • Improve its performance.
  • Update documentation.

In addition to this, I've also left a bunch of comments on this PR explaining the reason of most of the new changes.

👉 Demo page

@GP-Dan-Tovbein GP-Dan-Tovbein self-assigned this Aug 25, 2022
planet-4 added a commit to greenpeace/planet4-test-proteus that referenced this pull request Aug 25, 2022
/unhold 3707dcf7-85b7-44db-be6e-b9113701847e
@planet-4
Copy link
Contributor

planet-4 commented Aug 25, 2022

Test instance is ready 🚀

🌑 nix | admin | blocks report | CircleCI | composer-local.json

⌚ 2023.03.15 10:52:27

planet-4 added a commit to greenpeace/planet4-test-proteus that referenced this pull request Aug 25, 2022
/unhold a36cbd2d-2d09-4f53-80bf-e847df65cd5d
@GP-Dan-Tovbein GP-Dan-Tovbein force-pushed the PLANET-6874_gallery_block_hotfix branch from c30e13b to 8c46dcc Compare August 25, 2022 18:15
planet-4 added a commit to greenpeace/planet4-test-proteus that referenced this pull request Aug 25, 2022
/unhold a1f42112-1776-47fc-ac69-6c3402bf2a23
planet-4 added a commit to greenpeace/planet4-test-proteus that referenced this pull request Aug 25, 2022
/unhold e5ebe159-d312-4754-9bc4-6e449d3eeb3d
onClick={() => {
onImageClick(index);
}}
data-index={index}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this it will pass a data- value and retrieve it by doing this.

@@ -125,9 +125,8 @@ export const GalleryCarousel = ({ images, onImageClick, isEditing }) => {
style={{ objectPosition: image.focus_image }}
alt={image.alt_text}
title={image.alt_text}
onClick={() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically a performance tuning, since this lambda function is created every time the render method is invoked for the only reason to send the index value.

className,
gallery_block_style,
images,
attributes,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we hydrate the component.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could set {} as default value here? Then we would avoid later on having to check attributes && attributes.gallery_block_title for example 🤔

@@ -53,7 +53,7 @@ public function register_gallery_block() {
'render_callback' => static function ( $attributes ) {
$attributes['images'] = self::get_images( $attributes );

return self::render_frontend( $attributes );
return self::hydrate_frontend( $attributes );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to create a similar render_frontend method because when you hydrate a component you need to pass the data-hydrate instead of a data-render attribute. After we hydrate all render components we'll need to remove this method too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this affect how we implemented hydration for the Carousel Header? Maybe not in this PR, but we could open a follow-up ticket to investigate that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to check that, good suggestion :)

@GP-Dan-Tovbein GP-Dan-Tovbein marked this pull request as ready for review August 25, 2022 19:06
planet-4 added a commit to greenpeace/planet4-test-proteus that referenced this pull request Aug 26, 2022
/unhold 3c6ba4c6-d71c-43b7-8784-e90bba8f4218
planet-4 added a commit to greenpeace/planet4-test-proteus that referenced this pull request Aug 26, 2022
/unhold d8846611-0177-4391-8cc5-f60691bcfdd5
Copy link
Contributor

@mleray mleray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test the changes with existing blocks? On local I get the Attempt block recovery message in the editor and in the frontend the block no longer shows...

Edit: on the test instance, it seems that existing blocks still show on the frontend, but in the backend I do see the error message

@GP-Dan-Tovbein GP-Dan-Tovbein changed the title Planet 6874: Fix the Gallery block PLANET-6874: Fix the Gallery block Aug 26, 2022
@GP-Dan-Tovbein GP-Dan-Tovbein force-pushed the PLANET-6874_gallery_block_hotfix branch from 635d364 to 1e289a6 Compare August 30, 2022 17:51
@GP-Dan-Tovbein GP-Dan-Tovbein force-pushed the PLANET-6874_gallery_block_hotfix branch from 1e289a6 to b478237 Compare March 10, 2023 14:00
planet-4 added a commit to greenpeace/planet4-test-nix that referenced this pull request Mar 10, 2023
/unhold db51f8bd-c387-4d46-a6c8-0d559b82f2e5
@comzeradd comzeradd requested a review from mleray March 13, 2023 08:33
@mleray
Copy link
Contributor

mleray commented Mar 13, 2023

  • On local I see this message in the console:
Warning: Expected server HTML to contain a matching <section> in <div>.
    at section
    at GalleryFrontend
  • I still have the Hammer not defined error message sometimes, it seems to be when creating a new Gallery block with Slider style and previewing the page (without saving first) or when purging the page

I still see both of these issues 😕

@mleray
Copy link
Contributor

mleray commented Mar 13, 2023

I get a similar warning for the CarouselHeader, which we also use hydration for:

Screenshot 2023-03-13 at 10 16 30

planet-4 added a commit to greenpeace/planet4-test-nix that referenced this pull request Mar 14, 2023
/unhold 4a86ad40-767e-4959-9c64-895b4c2edf10
@GP-Dan-Tovbein GP-Dan-Tovbein force-pushed the PLANET-6874_gallery_block_hotfix branch from 1bb65b2 to f93c68c Compare March 14, 2023 13:21
planet-4 added a commit to greenpeace/planet4-test-nix that referenced this pull request Mar 14, 2023
/unhold 6a7200b4-4f8b-4e77-b03c-3a5d3c034e80
@GP-Dan-Tovbein
Copy link
Contributor Author

I get a similar warning for the CarouselHeader, which we also use hydration for:

Screenshot 2023-03-13 at 10 16 30

The improvement from the current PR will potentially fix the rest of blocks with the same error. That happens because the server store empty content which is completely different of what the frontend renders.

- Implement hydrate_frontend render callback function
- Make improvements to the gallery class, update attrs
- Pass new CSR attribute if it's needed
- Update documentation
- Modify openLightbox method passing the event instead an index
- Hydrate the component and deprecate againts the frontendRendered
- Implement new component version
- Implement validations
@GP-Dan-Tovbein GP-Dan-Tovbein force-pushed the PLANET-6874_gallery_block_hotfix branch from f93c68c to a71244a Compare March 14, 2023 14:45
planet-4 added a commit to greenpeace/planet4-test-nix that referenced this pull request Mar 14, 2023
/unhold ac010092-106b-49ad-9037-40ff67e0cb29
@GP-Dan-Tovbein GP-Dan-Tovbein force-pushed the PLANET-6874_gallery_block_hotfix branch 2 times, most recently from 045b681 to b2fd6e7 Compare March 14, 2023 22:22
planet-4 added a commit to greenpeace/planet4-test-nix that referenced this pull request Mar 15, 2023
/unhold 02d4c4ba-a359-42d7-91ab-132f594057ec
@GP-Dan-Tovbein GP-Dan-Tovbein force-pushed the PLANET-6874_gallery_block_hotfix branch from b2fd6e7 to 7742a5a Compare March 15, 2023 09:05
planet-4 added a commit to greenpeace/planet4-test-nix that referenced this pull request Mar 15, 2023
/unhold 8ea2e162-0a2e-4557-bb09-59619fece015
Copy link
Contributor

@mleray mleray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, great job! 👏 I've left 2 suggestions in the comments, but they're not blockers 🙂

- Modify variables assignements
- Update the deprecated attribute
- Fix the 'Warning: Expected server HTML to contain a matching <section> in <div>' issue
- Fix evt prevent default issue
@GP-Dan-Tovbein GP-Dan-Tovbein force-pushed the PLANET-6874_gallery_block_hotfix branch from 7742a5a to dad5c6e Compare March 15, 2023 10:11
planet-4 added a commit to greenpeace/planet4-test-nix that referenced this pull request Mar 15, 2023
/unhold 6cbfac0a-7c9c-4d35-8b71-032f81f1b0a6
planet-4 added a commit to greenpeace/planet4-test-nix that referenced this pull request Mar 15, 2023
/unhold e014cca4-acd2-4c80-bb71-916a3cdb2156
@GP-Dan-Tovbein GP-Dan-Tovbein merged commit 08b64ce into main Mar 15, 2023
2 checks passed
@GP-Dan-Tovbein GP-Dan-Tovbein deleted the PLANET-6874_gallery_block_hotfix branch March 15, 2023 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants