-
Notifications
You must be signed in to change notification settings - Fork 2k
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
FSE: Update blocks categories #43670
FSE: Update blocks categories #43670
Conversation
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. |
@@ -31,7 +32,7 @@ addFilter( | |||
registerBlockType( blockName, { | |||
...settings, | |||
title: __( 'Blog Posts', 'full-site-editing' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 80 times:
translate( 'Blog Posts' )
ES Score: 10
See 1 additional suggestions in the PR translation status page
Caution: This PR affects files in the FSE Plugin on WordPress.com D45465-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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting on this! I know it's a draft, I left some early thoughts.
apps/full-site-editing/full-site-editing-plugin/block-helpers.js
Outdated
Show resolved
Hide resolved
apps/full-site-editing/full-site-editing-plugin/block-helpers.js
Outdated
Show resolved
Hide resolved
apps/full-site-editing/full-site-editing-plugin/block-helpers.js
Outdated
Show resolved
Hide resolved
export const hasLegacyCategory = getCategories().some( function ( category ) { | ||
return category.slug === 'formatting'; | ||
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a capability check you use to build conditionals, we wrap the conditional logic and accept a list of candidate categories?
function getCategoryWithFallbacks( ...requestedCategories: string[]): string {
const knownCategories: Array< { slug: string } > = getCategories();
for ( const requestedCategory of requestedCategories) {
if ( knownCategories.some( ( { slug } ) => slug === requestedCategory ) ) {
return requestedCategory;
}
}
// Should we throw an error here? 🤔
throw new Error('Terrible choice of category 🤦♀️');
}
// Usage:
registerBlockType( 'my-block/name', { category: getCategoryWithFallbacks( 'layout', 'widgets' ) } );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a nice idea! Avoiding having to write a ternary expression would be nice, indeed. Just wondering how we could make this function re-usable across all packages that register blocks. We'll probably need to duplicate it for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we'll need to write it once for each package (FSE, Jetpack…).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I really like this approach as it's basically future proof. If we change categories again, then it's just a matter of passing it as the last one in the argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it makes sense to move this function over to @wordpress/blocks
... 🤔 this way, we'd avoid the current repetition we have across different block repos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that idea. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it makes sense to move this function over to @wordpress/blocks... 🤔 this way, we'd avoid the current repetition we have across different block repos.
IIUC, if we need this to handle new categories on sites where Gutenberg has not been updated yet (as it handles the other way round automagically), we can't actually move it to a Gutenberg package, because on those sites that utility won't be available. AFAIR, Jetpack imports use packages that are currently available with the installed version of the Gutenberg plugin, not those that are defined in package.json
(i.e. @worpdress/blocks
). Pinging @jeherve here for help 🙏
If the above is true and we end up dropping the Gutenberg PR, how about we make it into a Calypso package instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, if we need this to handle new categories on sites where Gutenberg has not been updated yet (as it handles the other way round automagically), we can't actually move it to a Gutenberg package, because on those sites that utility won't be available
But isn't the @wordpress/blocks
a package that can be installed separately (even though it lives within the Gutenberg repo)? Right now, we need this function in the following packages:
- https://github.com/Automattic/wpcom-blocks/pull/178
- Update blocks categories jetpack#16363
- FSE: Update blocks categories #43670
- Update blocks categories block-experiments#110
Wouldn't it just be a matter of publishing a new version of @wordpress/blocks
and then updating it on these packages?
I'm fine abandoning that PR and moving it to a Calypso package if we find it's the best way to go, by the way :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry I read too fast, you actually mention here:
AFAIR, Jetpack imports use packages that are currently available with the installed version of the Gutenberg plugin, not those that are defined in package.json (i.e. @worpdress/blocks). Pinging @jeherve here for help
Let's wait for @jeherve's insights about that, then :)
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we considered also adding a linting rule that checks the category in warning about new categories as well? Something like checking the category of the registerBlockType()
and warning if we know it's a category that's not going to work with older gutenbergs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, I left a few comments.
This commit has been added but probably will not be included as part of the PR. It's not related to the changeset but was required for me to get my wp-calypso env running.
It looks like there are a few changes to the setup here, it may make sense to break those out into an independent PR. There are multiple concerns here, but the changes are small enough that it's reviewable. At your discretion either way 👍
Should we write tests to check that each block is assigned to the right category names depending on the set of categories? Not sure what is the testing policy for Calypso and what coverage we aim for.
Let's stay focused on testing just the added utility function you added. If we start testing the block registration, we quickly get outside of unit tests.
e2e test may make sense at some point to ensure blocks are in the expected categories, but that's not the scope we're looking at here.
apps/full-site-editing/full-site-editing-plugin/block-helpers/block-helpers.test.ts
Outdated
Show resolved
Hide resolved
apps/full-site-editing/full-site-editing-plugin/block-helpers/block-helpers.ts
Outdated
Show resolved
Hide resolved
8308272
to
f686f8c
Compare
a5d4c76
to
a9f82b2
Compare
apps/full-site-editing/full-site-editing-plugin/newspack-blocks/blog-posts-block-editor.js
Outdated
Show resolved
Hide resolved
c9a4df3
to
8ba7810
Compare
I decided to revert my changes to the premium block categories because:
|
Yeah, I'd also just rely on stakeholder team(s) on those blocks and leave them be if they so wish. FYI @lessbloat. 👋 We can consider this unblocked and proceed with the rest of the blocks then? :-) |
94a1eea
to
3089e0c
Compare
Hi Mikael 👋
I changed all the blocks under FSE I could find (the ones that use The PR for Jetpack is here: Automattic/jetpack#16363. These are the two outstanding group of blocks that need to be dealt with according to the original issue. Apart from Jetpack and FSE (and Coblocks), what other apps/libs/packages have blocks that need renaming? |
3ec65b0
to
f2bf6c0
Compare
Thanks everyone for helping out!
For the remaining
For reference, the valid categories are: [
{ slug: 'text', title: __( 'Text' ) },
{ slug: 'media', title: __( 'Media' ) },
{ slug: 'design', title: __( 'Design' ) },
{ slug: 'widgets', title: __( 'Widgets' ) },
{ slug: 'embed', title: __( 'Embeds' ) },
] |
They are already mapped to these categories: |
Summary of block changes mostly for myself 🙂
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small changes, I'll go ahead and apply these…
apps/full-site-editing/full-site-editing-plugin/block-helpers/index.ts
Outdated
Show resolved
Hide resolved
301b4c8
to
c778506
Compare
I applied a few changes to align dependency versions across the monorepo. I also rebased. |
7077925
to
e043acf
Compare
@noahtallen It looks like this is getting a false negative on the phpcs action It's trying to run phpcs on a file that was added and deleted in this PR, which errors:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on Atomic. I see the blog posts and post carousel blocks in the updated categories. I think this is good to go 👍
I'd like to get #43670 (comment) sorted out, but I think that's a false negative and unlikely to affect this after merge.
@sirreal I think this makes sense. The issue is that we run phpcs on all "changed" files, which includes deleted files. Will have to adjust that logic to only handle added or changed files |
This diff should do it (feel free to commit to this PR to test it) diff --git a/.github/workflows/full-site-editing-plugin.yml b/.github/workflows/full-site-editing-plugin.yml
index c49652d16a..c794789311 100644
--- a/.github/workflows/full-site-editing-plugin.yml
+++ b/.github/workflows/full-site-editing-plugin.yml
@@ -133,11 +133,8 @@ jobs:
with:
token: ${{ secrets.GITHUB_TOKEN }}
- - name: Displaying changed files
- run: echo ${{ join( fromJson( steps.changes.outputs.all ) ) }}
-
- name: Execute phpcs on changed files
- run: ./vendor/bin/phpcs --standard=apps/phpcs.xml ${{ join( fromJson( steps.changes.outputs.all ), ' ' ) }}
+ run: ./vendor/bin/phpcs --standard=apps/phpcs.xml ${{ join( fromJson( steps.changes.outputs.modified ), ' ' ) }} ${{ join( fromJson( steps.changes.outputs.added ), ' ' ) }}
if: ${{ steps.changes.outputs.all != '' }}
- name: No changes found
ddd |
5bd5e9e
to
15992ef
Compare
15992ef
to
6b2e38a
Compare
@Automattic/good-mountain Fellows, I ended up getting into a 🐇 🕳️ and couldn't deploy today. But it was for a good cause (hopefully!). It's related to the previous report I made here. More details below. It seems some earn (premium) blocks were broken for a while.While testing on Atomic sites, we've found the following:
The difference when testing with this branch's FSE is that in older Gutenberg versions where the It seems these issues are independent of the changes added here. Still, the changes here make the two "problematic" premium blocks appear on Gutenberg versions < @deBhal is gardening, and this bug probably falls on gardeners' plate. We spent some time pairing on this today, and he's still looking into it as I end my day now. 🇦🇺 I could have managed to deploy this today, but I wanted to wait to understand what it's about and if we need to do anything in the category side. I'm pretty sure that this can be merged and perhaps deployed tomorrow. *With the help of @deBhal, who kindly offered to help me deploy this PR and paired with me on this issue, thanks a lot! |
Conversation on handling broken blocks: pbAok1-1ez-p2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for shepherding the update @fullofcaffeine! This tests well for Premium Content blocks on Simple Sites (edge and 8.3).
@Automattic/serenity is looking into potential breakages into Atomic from Premium Content, but it shouldn't affect this PR. We'll follow up shortly.
Context: #43198. - Also add a helper function for FSE blocks that can be used to check if old category names exist in the store and fall-backs to an older version(s) otherwise. This function should be used when the updated category is a completely new category being introduced by a new version of Gutenberg. You then pass this new category + the older categorie(s) that will be tried in sequence if the previous one was not found. - Tweaked the FSE Jest configuration to support TypeScript.
…ontent The fallback was causing these blocks to appear in older versions of Gutenberg where the `design` category was not available. Until here, all good. However, they were all er'ing upon insertion. We thought it would be better to rollback this change for now.
0bd07b6
to
4e0ec42
Compare
Changes proposed in this Pull Request
Related PRs
Categories
a8c/blog-posts
fromlayout
->widgets
;a8c/posts-carousel
fromlayout
->widgets
*[0]a8c/template
fromlayout
->design
*[1]a8c/navigation-menu
fromlayout
->design
*[1]a8c/post-content
fromlayout
->design
*[1]a8c/site-title
fromlayout
->design
*[1]a8c/site-credit
fromlayout
->design
*[1]a8c/site-description
fromlayout
->design
*[1]a8c/donations
fromcommon
->widgets
*[0]jetpack/event-countdown
in thewidgets
categoryjetpack/timeline
in thewidgets
categoryjetpack/timeline-item
in thewidgets
categorypremium-content/button
fromlayout
->design
premium-content/login-button
fromlayout
->design
premium-content/container
fromcommon
->design
premium-content/logged-out-view
fromcommon
->design
premium-content/subscriber-view
fromcommon
->design
*[0] This was a guess, let me know if this is the right category to move it to.
*[1] I did set the category name for those using our helper function, even though I understand that a migration already took care of automatically mapping
layout
todesign
if I'm not mistaken. Let me know if you think it would not be needed for those cases.TODO
Outstanding questions
posts-list-block
? (More details in p1593038635493700-slack-cylon)Testing instructions
Atomated tests
cd
towp-calypso/apps/full-site-editing
and runyarn test:js
.To manually test the changes
cd
towp-calypso/apps/full-site-editing
and runyarn dev --sync
to sync it to your sandbox.Exford
)Test the scenarios above in the following environments:**
Note: If this PR gets approved and merged, then I'll remove the helper function and tests from here (including the installation/configuration of jest).