-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add reusable block tab to inserter. #23296
Conversation
Size Change: +181 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
b0c1435
to
0dd3b6d
Compare
This works well. My only suggestion from a design perspective is to change the label, as @mtias suggested, to "Saved." |
Does that make sense? It seems weird and inconsistent to call them "Saved" in one spot but "Reusable" everywhere else. |
0dd3b6d
to
00125c4
Compare
I tend to lean toward sticking with "Reusable" for consistency -- the existence/idea of reusable blocks is still new, so I'd want people to be confident in their ability to find what they need by looking for the "reusable" language that we use elsewhere. |
00125c4
to
94a0e3d
Compare
I think we should rename to Saved across the board if we lift it up as its own grouping (which I like). I've seen Reusable to suffer from lack of context (reusable blocks is much better than reusable) and translation shortcomings in a few languages. Saved stands on its own a bit better next to Patterns and Blocks. That said, we should look at renaming separately and review the different contexts in which we show the action, as well as the icon. |
packages/block-editor/src/components/inserter/reusable-block-list.js
Outdated
Show resolved
Hide resolved
categories, | ||
collections, | ||
filterValue | ||
).filter( ( { category } ) => category === 'reusable' ); |
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.
Not for this PR, but I think we should let go of "reusable" being a category and instead allow the tab to display other categories. That would allow a site to categorize reusable blocks (with the regular category interface when managing the custom post type) and have the categories listed in this tab as regular block categories.
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.
Agreed.
94a0e3d
to
af3e07d
Compare
Tested this PR on Safari + VoiceOver. The tabs are properly announced as before, both their name and the number of tab (ie. 3 of 3), as well as the number of blocks under each one (with the exception of Patterns). From an accessibility perspective, this looks good to me 👍 |
af3e07d
to
689e38a
Compare
After #22789 got merged, I needed to rebase this. I did, and now the reusable blocks tab isn't showing up. I'll let you know when I fix it. |
a6a2134
to
8ea92db
Compare
bc9d192
to
6cb6761
Compare
Alright, I've spent the whole day working on fixing the end-to-end tests. I'm down to just one failing test, and I can't figure out why it's failing:
It seems like the Reusable block tab is being added (causing the inserter to use tabs, and thus causing an extra Tab press to be necessary), but if that's the case (and it might not be), I don't know what's causing the Reusable block tab to appear. I need help to figure this one out. |
I took a look at this, and tests pass for me locally. I even tried this interactive test:
and the failing test in question appears to work as intended visually too. Here's the test bit:
So, click the inserter, search for "gallery", press tab twice, and you should've inserted a gallery block. This is where it gets interesting, because regardless of configuration, this always takes 3 tab presses for me. Here's your branch with no reusable blocks: Here's your branch, with a reusable block: Here's the master branch with a reusable block: In all three, you press tab 3 times. Which begs the questions:
@youknowriad — Jorge wrote these tests but is out. Do you have any insights here? |
packages/block-editor/src/components/inserter/reusable-blocks-tab.js
Outdated
Show resolved
Hide resolved
|
||
assertNoResultsMessageNotToBePresent( container ); | ||
} ); | ||
} ); |
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 not really certain this unit tests is very valuable personally but your call.
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 didn't create these tests from scratch. They were originally in the unit test file for the standard blocks tab. I just moved them over since reusable blocks are now in their own tab.
packages/e2e-tests/specs/editor/various/reusable-blocks.test.js
Outdated
Show resolved
Hide resolved
I think the test failure comes from the fact that if you have reusable blocks, you'll have an extra tab stop on that inserter. |
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.
Cool stuff, this is looking good, I left some minor comments and suggestions about the e2e tests errors.
6cb6761
to
72e8847
Compare
Small tip: instead of amending commits (sometimes it's necessary though), it's better to keep pushing new commits in PRs. This makes it easier for folks following the PR to know what changed in the latest commits. |
72e8847
to
35f785a
Compare
packages/block-editor/src/components/inserter/reusable-blocks-tab.js
Outdated
Show resolved
Hide resolved
I didn't realize reusable blocks were persisted across tests. Is there a simple way to delete all reusable blocks at the end of a test? Or do you have to perform the deletion "manually" (by opening the more menu on each reusable block and clicking the "Remove from Reusable blocks" option). |
@ZebulanStanphill There's a |
1406d6b
to
c4b6798
Compare
c4b6798
to
dad3772
Compare
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.
Let's get this in to be ready for 5.5 beta 1.
💯 kudos here @ZebulanStanphill
👌 |
Thanks for fixing the tests, @youknowriad! 😄 |
Description
closes #22860.
closes #23253
This PR moves reusable blocks from a category mostly hidden at the bottom of the standard blocks tab in the inserter to their own tab.
Currently, it uses the style of the block tab for presenting the reusable blocks. But I think it may make more sense to present them the same way as patterns. Let me know what you think in #22860.
How has this been tested?
I tested to make sure the tab showed reusable blocks, that inserting reusable blocks worked, and that the reusable category was removed from the standard blocks tab.
Screenshots
Checklist: