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 FSE plugin to version to 1.13 #44125

Merged
merged 4 commits into from
Jul 23, 2020
Merged

Conversation

deBhal
Copy link
Contributor

@deBhal deBhal commented Jul 13, 2020

Changes proposed in this Pull Request

Version bump and changelog.

Testing instructions

This deploy includes quite a number of PRs, so I'm going to ask as many authors as possible to quickly re-test their changes against the release candidate to make sure there are no surprising interactions.

The detailed instructions are in the field-guide here:

But if you just need the short version:

  • Apply code-D46328
  • Test on a sandboxed simple-site with appropriate FSE-plugin flags
  • Test on an atomic site with and without Gutenberg enabled

Here's the rc plugin link for your convenience fse-build-archive (2.23 MB) (updated to include fix by @akirk)

Please also add any comment you'd like included in changelog here, and I'll add them in before I merge

Changelog entries

  • Change the category of FSE blocks from legacy to the updated ones (Updated default block categories for Automattic blocks #43198).
  • Add a helper function that can be used to assign categories with older fallbacks.
  • Add support for TypeScript tests.
  • Update visual style of navigation sidebar.
  • Fix navigation sidebar dismiss button in IE.

PRs to test

Final pre-release checks

  • Confirm phpcbf fixes occur in the Github action build log (Should be in Build FSE plugin/Build packages
    Update_FSE_plugin_to_version_to_1_13_by_deBhal_·Pull_Request__44125·_Automattic_wp-calypso

@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

Caution: This PR affects files in the FSE Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D46328-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@deBhal deBhal marked this pull request as draft July 14, 2020 03:45
@deBhal deBhal added [Goal] Gutenberg Working towards full integration with Gutenberg [Status] Blocked / Hold DO NOT MERGE labels Jul 14, 2020
@deBhal deBhal self-assigned this Jul 14, 2020
@deBhal deBhal force-pushed the update/fse-plugin-to-v-1-13 branch from dddf3ee to a455c42 Compare July 17, 2020 01:57
@deBhal
Copy link
Contributor Author

deBhal commented Jul 20, 2020

#43670 is merged and #44191 fixes #44160, so we're clear to go ahead now.

@deBhal deBhal changed the title Update FSE plugin to version version to 1.13 Update FSE plugin to version to 1.13 Jul 20, 2020
@deBhal deBhal force-pushed the update/fse-plugin-to-v-1-13 branch from a455c42 to cbf47d3 Compare July 20, 2020 10:09
@deBhal deBhal marked this pull request as ready for review July 20, 2020 10:35
@deBhal deBhal requested a review from a team as a code owner July 20, 2020 13:59
@akirk
Copy link
Member

akirk commented Jul 20, 2020

@deBhal I have added a commit to this PR that fixes the error:

@automattic/full-site-editing: ./bin/sync-newspack-blocks.sh: line 132: ../../vendor/bin/phpcbf: No such file or directory
@automattic/full-site-editing: phpcbf: !! There was an error executing phpcbf

I last fixed this here: 0cfd9a3 but it looks like in recent builds the "Build packages" step was skipped in favor of "Build dependencies" (Checks log 1.11.0 vs Checks log 1.12.0), I am observing the same in this PR.

This caused the textdomains not to be rewritten to full-site-editing as anticipated, so I hope to have fixed this again and for a longer period :)

Could you please ensure that you see something like @automattic/full-site-editing: phpcbf: A TOTAL OF 77 ERRORS WERE FIXED IN 5 FILES in the Github Action build log before you ship FSE 1.13? Thanks!

@akirk akirk force-pushed the update/fse-plugin-to-v-1-13 branch from 0a61de8 to bfc4bff Compare July 20, 2020 14:02
@noahtallen
Copy link
Member

so I hope to have fixed this again and for a longer period :)

🤦 Sorry that we didn't catch this again. Would it be helpful to add a unit test for this so PRs fail when the textdomain in newspack blocks does not get updated?

@kwight
Copy link
Contributor

kwight commented Jul 20, 2020

@deBhal Serenity's pieces are good to go; we've got some additional followup, but nothing that results in fatals or that should hold up pushing out this version. 👍

Copy link
Member

@p-jackson p-jackson left a comment

Choose a reason for hiding this comment

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

I've smoke tested the sidebar changes to confirm they're working correctly in this release.

Also checked that the release related changes look good so take this ✅

@akirk
Copy link
Member

akirk commented Jul 21, 2020

@noahtallen How would you approach this? We are talking about files that are not committed, just shipped in the ZIP file, plus I'm not sure we can be certain about which files exist, newspack blocks could change their file layout.

@deBhal
Copy link
Contributor Author

deBhal commented Jul 21, 2020

@akirk: I created #44296 with a simple suggestion for fixing the textdomain issue

@sarayourfriend
Copy link
Contributor

Hi @deBhal. My changes are just to CircleCI so can be ✅; no actual changes to the plugin.

@noahtallen
Copy link
Member

noahtallen commented Jul 21, 2020

We are talking about files that are not committed, just shipped in the ZIP file

True... though they should still exist in the local filesystem for any test purposes, so long as we are testing against a "built" version of the plugin! Some ideas...

  1. move the check to the build step, and then allow the build step to fail if the textdomain was not translated
  2. just test against a fairly stable file, like class-newspack-blocks.php (though I guess that could change to not have any translated strings in it 🤔 )
  3. Add a special lint-check in GitHub CI which runs phpcs on full-site-editing-plugin/newspack-blocks-synced-newspack-blocks using the build artifact, and only fails if the textdomain rule fails

Edit: see this PR for my proposed solution: #44323

@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 21, 2020
@fullofcaffeine
Copy link
Contributor

Tested on an FSE-enabled simple site and it looks good. Took into account the changes from Serenity.

In AT sites I couldn't fully test it though. There appears to be a bug preventing FSE from enabling on AT sites, see: #41825. Discussion: p1595373248144700-slack-cylon.

@deBhal
Copy link
Contributor Author

deBhal commented Jul 22, 2020

@fullofcaffeine: I forced the dotcom-fse on on my ephemeral site (see #41825) and tested this release + dotcom-fse (these names are so confusing :/) with and without gutenberg enabled, and it all looks good, so I'm going to go ahead and tick that off:

Add_New_Page_‹My_WordPress_Site—_WordPress

@georgeh
Copy link
Contributor

georgeh commented Jul 22, 2020

The Timeline block changes look good on wpcom simple and Atomic

@deBhal
Copy link
Contributor Author

deBhal commented Jul 23, 2020

Ok, last check is in.

Those signup timeout failures in the e2e tests are known, and not related to this diff, so we're going ahead.

@sirreal
Copy link
Member

sirreal commented Jul 23, 2020

👋 I've just landed #44357, can I pull it in here?

@sirreal sirreal force-pushed the update/fse-plugin-to-v-1-13 branch from 8f35981 to 105485f Compare July 23, 2020 08:00
@sirreal
Copy link
Member

sirreal commented Jul 23, 2020

I've rebased to pull in #44357.

@deBhal
Copy link
Contributor Author

deBhal commented Jul 23, 2020

I've rebased to pull in #44357.

Ok, I'm going to trust you know what you're doing, and just do an additional smoke test.

@deBhal deBhal merged commit 78ce399 into master Jul 23, 2020
@deBhal deBhal deleted the update/fse-plugin-to-v-1-13 branch July 23, 2020 11:43
@matticbot matticbot removed [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Gutenberg Working towards full integration with Gutenberg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants