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

fix(carousel): avoid editor crash on empty #2058

Merged
merged 1 commit into from
Feb 19, 2025
Merged

Conversation

naxoc
Copy link
Member

@naxoc naxoc commented Feb 18, 2025

All Submissions:

Changes proposed in this Pull Request:

This fixes a bug where the Content Carousel block would "crash" in the editor and just show a spinner and not be responsive to changes made from the sidebar.

How to test the changes in this Pull Request:

Before applying the patch, trigger the bug:

  1. Create a Content Carousel on a post in the editor
  2. In the "Content" panel, set a post category or tag - I just used "Science"
  3. In the "Post Types" panel, select a different post type in addition to "Posts" that can't have any post categories or tags – I just used "Page"
  4. Deselect "Posts" and observe editor crash and JS console error

Now apply the patch:

  • Check that toggling between having post and page selected under post type will not crash the block
  • Check that the "Sorry, no posts were found." text is displayed when only page is selected.
  • Check that there are no console errors
  • Check that the block behaves as expected (shows the content you asked it to).

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@naxoc naxoc requested a review from a team as a code owner February 18, 2025 17:13
@naxoc naxoc self-assigned this Feb 18, 2025
@@ -34,7 +34,7 @@ function newspack_blocks_render_block_carousel( $attributes ) {

$article_query = new WP_Query( Newspack_Blocks::build_articles_query( $attributes, apply_filters( 'newspack_blocks_block_name', 'newspack-blocks/carousel' ) ) );
if ( false === $article_query->have_posts() ) {
return;
return '';
Copy link
Member Author

Choose a reason for hiding this comment

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

The docs said it return string, so I changed that to ''.

Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Works well and avoids the crash!

@naxoc naxoc merged commit c316801 into trunk Feb 19, 2025
10 checks passed
@naxoc naxoc deleted the fix/carousel-zero-posts branch February 19, 2025 09:48
Copy link

Hey @naxoc, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

matticbot pushed a commit that referenced this pull request Feb 20, 2025
# [4.7.0-alpha.1](v4.6.0...v4.7.0-alpha.1) (2025-02-20)

### Bug Fixes

* **carousel:** avoid editor crash on empty ([#2058](#2058)) ([c316801](c316801))
* **modal-checkout:** better iframe sizing ([#2052](#2052)) ([aa308f2](aa308f2))
* **recaptcha:** use clone of #place_order button to trigger checkout ([#2028](#2028)) ([46eb8b5](46eb8b5)), closes [#2030](#2030) [#2030](#2030)

### Features

* add styles to fix Braintree modal appearance ([#2036](#2036)) ([9ab2c62](9ab2c62))
* add toggle for transaction details ([#2049](#2049)) ([d254aca](d254aca))
* **carousel:** rename block and reorganise settings ([#1962](#1962)) ([9905717](9905717))
* update blocks with new brand ([#2050](#2050)) ([2711302](2711302))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 4.7.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants