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

Update Newspack Blocks #51583

Merged
merged 10 commits into from
Jul 23, 2021
Merged

Update Newspack Blocks #51583

merged 10 commits into from
Jul 23, 2021

Conversation

adekbadek
Copy link
Member

@adekbadek adekbadek commented Mar 31, 2021

Changes proposed in this Pull Request

Updates Newspack Blocks to v1.30.0.

Automattic/newspack-blocks#725 changes how deduplication is handled.

Post Carousel also has several new options that affect how it renders on the front-end.

Testing instructions

  1. Ensure the new version of newspack-blocks is synchronised with FSE
  2. Create a page and insert both Blog Posts and Carousel blocks
  3. Observe that the deduplication works across all blocks - no posts are duplicated
  4. For the Post Carousel, observe the new options and confirm that they work as expected in the editor and on the front-end:

Screen Shot 2021-07-19 at 10 39 28 PM

  • Slide aspect ratio
  • Image fit
  • Number of slides to show at once
  • Post types (should allow posts and pages, at least, unless the site has plugins that register additional post types for the block)

@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

matticbot commented Mar 31, 2021

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~2907 bytes added 📈 [gzipped])

name                   parsed_size           gzip_size
entry-gutenboarding       +20732 B  (+1.0%)    +5764 B  (+1.0%)
entry-domains-landing     +20576 B  (+3.5%)    +5721 B  (+3.4%)
entry-main                 +9554 B  (+0.7%)    +2864 B  (+0.8%)
entry-login                +9554 B  (+1.0%)    +2864 B  (+1.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Async-loaded Components (~2417 bytes removed 📉 [gzipped])

name                                            parsed_size           gzip_size
async-load-design-wordpress-components-gallery      -7872 B  (-1.0%)    -2417 B  (-1.3%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@dkoo
Copy link
Contributor

dkoo commented Apr 6, 2021

@adekbadek Picking up the thread from Automattic/newspack-blocks#725.

I merged the latest trunk into this branch and synced the latest version of newspack-blocks code to my sandbox with updated paths. Now, the Blog Posts block works fine, but the Posts Carousel block crashes hard as soon as it's inserted:

Screen Shot 2021-04-06 at 11 18 30 AM

As you can see in the screenshot, the placeholder files in /assets are not being referenced from the right location either, but I haven't dug in yet to where they should go.

@github-actions
Copy link

github-actions bot commented Jun 5, 2021

This PR has been marked as stale due to lack of activity within the last 30 days.

@matticbot
Copy link
Contributor

matticbot commented Jul 15, 2021

@adekbadek
Copy link
Member Author

@dkoo I've rebased this branch onto trunk. When testing this branch of Calypso and syncing with the Blocks' master, I see no issues: a Homepage Posts block & Carousel block render fine in both editor and on the front-end, and have shared de-duplication. Could you please check again?

@matticbot
Copy link
Contributor

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit add/newspack-blocks-update on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

@dkoo
Copy link
Contributor

dkoo commented Jul 16, 2021

@adekbadek I can confirm that I too no longer see the error described in #51583 (comment), and that shared deduplication is working across Blog Posts and Post Carousel blocks. 🥳

However, the block attributes for Post Carousel seem to be breaking the SwiperJS functionality in the editor preview for me. This is when syncing the latest master of newspack-blocks to my sandbox. I'll see if I can find out more info on why.

EDIT: This is a bug in the Blocks repo. Fix is here: Automattic/newspack-blocks#807

@adekbadek
Copy link
Member Author

With the fix on Blocks ready, all that's left in this PR is the hardcoded version bump after the fix is released, right?

@dkoo dkoo added the Editing Toolkit For issues and PRs that affect the Editing Toolkit plugin label Jul 20, 2021
@adekbadek adekbadek marked this pull request as ready for review July 20, 2021 12:43
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 20, 2021
@dkoo
Copy link
Contributor

dkoo commented Jul 20, 2021

Tested in both sandbox and Atomic environments. edb0513 updates outdated file paths in the sync script, which caused the plugin to be missing files and throw fatals in the latter environment.

@matticbot
Copy link
Contributor

This PR modifies the release build for notifications

To test your changes on WordPress.com, run install-plugin.sh notifications add/newspack-blocks-update on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-elI-p2

@adekbadek adekbadek requested a review from a team as a code owner July 20, 2021 21:34
@dkoo
Copy link
Contributor

dkoo commented Jul 20, 2021

To test your changes on WordPress.com, run install-plugin.sh notifications add/newspack-blocks-update on your sandbox.

I ran this on my sandbox and tested the updated build successfully.

@dkoo dkoo force-pushed the add/newspack-blocks-update branch from 950acfe to b5ab7a2 Compare July 22, 2021 19:30
@dkoo
Copy link
Contributor

dkoo commented Jul 22, 2021

Rebased on trunk and re-tested in both sandbox and Atomic. Everything looks 👍 to me, so I think this is ready for a merge, now!

Copy link
Contributor

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

The changes here look good to me. The only thing a bit odd is the addition of @wordpress/elements v3, though since all the builds pass (and smoke tests work well), this should not be problematic. It originates in the new dependency on @wordpress/icons in newspack blocks.

I'll note that I didn't test the newspack changes on my sandbox though. @dkoo I think a new version of Gutenberg was released to wpcom in the past 12 hours. I think it's live for simple sites, but not yet for Atomic. It would probably be worth re-testing it in those environments :)

@dkoo dkoo force-pushed the add/newspack-blocks-update branch from b5ab7a2 to a1caf1c Compare July 23, 2021 18:14
@dkoo dkoo merged commit 9cbe7e4 into trunk Jul 23, 2021
@dkoo dkoo deleted the add/newspack-blocks-update branch July 23, 2021 19:15
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editing Toolkit For issues and PRs that affect the Editing Toolkit plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants