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

Slideshow: do not render markup when user doesn't select any images #16231

Merged

Conversation

cpapazoglou
Copy link
Contributor

@cpapazoglou cpapazoglou commented Jun 22, 2020

This PR resolves #15273, where adding a slideshow block, without selecting any image, results to obsolete markup in the frontend. Especially, in the case of AMP rendering there was an extra visible markup.

Changes proposed in this Pull Request:

  • Return null when images array is empty so that no obsolete markup is rendered both for normal and AMP rendering

Jetpack product discussion

Does this pull request change what data or activity we track or use?

No

Testing instructions:

  • Setup local jetpack env with ngrok tunneling as per instructions
  • Install the AMP plugin By AMP Project Contributors here /wp-admin/plugin-install.php?s=amp&tab=search&type=term
  • Go to /wp-admin/post-new.php
  • Add a slideshow block AND do not select any image
  • Save and visit the above post
  • Visit the the same post by appending /amp to the URL to inspect how AMP renders

Before
CleanShot 2020-06-22 at 16 34 02

After
CleanShot 2020-06-22 at 16 36 09

Before AMP
CleanShot 2020-06-22 at 16 37 13

After AMP
CleanShot 2020-06-22 at 16 39 02

Proposed changelog entry for your changes:

  • Slideshow: do not render markup when user doesn't select any images

Notices

The above changes will work for:

  • new content both for normal and AMP rendering
  • old content for AMP rendering

The above changes will not work for:

  • old content for normal rendering

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello cpapazoglou! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D45278-code before merging this PR. Thank you!
This revision will be updated with each commit to this PR

@jetpackbot
Copy link
Collaborator

jetpackbot commented Jun 22, 2020

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16231

Scheduled Jetpack release: July 7, 2020.
Scheduled code freeze: June 30, 2020

Generated by 🚫 dangerJS against aff2683

@frontdevde frontdevde self-requested a review June 22, 2020 14:16
@jeherve jeherve added [Block] Slideshow [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Jun 22, 2020
Copy link
Contributor

@frontdevde frontdevde left a comment

Choose a reason for hiding this comment

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

Thanks for swiftly addressing the feedback!

I tested as per testing instructions and can confirm that the Slideshow isn't rendered anymore when no images are selected. This is true for both the normal and AMP view.

LGTM 👍

@frontdevde frontdevde added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Team Review labels Jun 24, 2020
@cpapazoglou cpapazoglou added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jun 24, 2020
@jeherve jeherve added this to the 8.7 milestone Jun 25, 2020
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jun 25, 2020
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This tests well for me, it should be good to merge! 🚢

@cpapazoglou cpapazoglou merged commit 3b5cf19 into master Jun 25, 2020
@cpapazoglou cpapazoglou deleted the fix/do-not-render-markup-for-a-slideshow-without-images branch June 25, 2020 08:55
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jun 25, 2020
@cpapazoglou
Copy link
Contributor Author

r209584-wpcom

jeherve added a commit that referenced this pull request Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Slideshow Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slideshow block: Don't output anything if the block is empty
7 participants