Remove the verb Toggle from the Block Inserter button. #65983
Conversation
afercia
requested review from
ntwb,
nerrad,
ajitbohra,
draganescu,
adamziel and
kevin940726
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
draganescu
added
[Type] Enhancement
There was a problem hiding this comment.
Code wise it's a string replacement PR, let's wait for validation on the change itself. Otherwise great! I do agree with toggle being untranslateable but replacing it with just what the button is and not what it does makes me think maybe there is a better way? IDK tbh.
|
Size Change: +8 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Hello! 👋 Editor here, answering the "needs copy review" ping.
^ No concerns with this proposed change. |
|
Thanks for adding the missing issue labels. Looking into the failing tests. I'd agree the name itself would needs some rethinking. This PR is just aiming to remove the verb 'toggle'. |
|
Flaky tests detected in dd3dab9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11256083346
|
|
I'm not sure I understand why the Performance test is failing |
|
I believe the problem wiht the failing tests is that there are two buttons matching the selector "Block Inserter": The actual "Block Inserter" and "Close Block Inserter". To solve that, you can pass the property const globalInserterToggle = page.getByRole( 'button', {
name: 'Block Inserter',
exact: true,
} ); |
|
Thanks @SantosGuillamot I did try |
|
I pushed a commit that attempts to solve the performance test. In this PR, we expect the global inserter to have a label of "Block Inserter" in the But when the global inserter is open, there are two buttons that match this name. Screenshot:
So it seems like the error is happening here: So I added a new I hope this approach solves the test, but I am concerned that this change might affect metrics at the same time. |
|
When I run that sprecific test locally, the Gutenberg plugin gets |
From what I understand, the performance test runs Furthermore, if there is no operation to explicitly close the main inserter in the e2e test, I think
Have you specified a specific WordPress version via Normally, the latest version of WordPress should be enabled in both environments ( If there is no improvement, try |
Nope.
I'm not sure that is the case. As far as I can tell, the WordPress images of the Gutenberg wp-env need to be updated regularly. |
Have you tried that? Either way, if you're really having problems with wp-env, it's best to open a new issue. |
|
@t-hamano thanks, I appreciate your feedback. I think I prefer to wait for an answer from the contributors I pinged on this PR. Thanks. |
|
I'd appreciate a review to unblock this PR, thanks. Cc @WordPress/gutenberg-core |
Changes to the performance test code will need to be made at some points, so I expect it's ok. @youknowriad or @tyxla might know a bit more. |
When this happens to me I kill the images For what it's worth, I tested this PR and things LGTM
I was wondering: when the block inspector is activated, is the button's |
There was a problem hiding this comment.
So I added a new getByRole to target only the button in the header.
I hope this approach solves the test, but I am concerned that this change might affect metrics at the same time.Changes to the performance test code will need to be made at some points, so I expect it's ok. @youknowriad or @tyxla might know a bit more.
While I'd generally say this should be fine because performance tests change every once in a whiile, it's definitely something to consider. Adding a .getByRole() could potentially increase the test by a few ms, which is a lot for a test that takes ~60ms on average. It might make sense to consider other ways to solve the ambiguity with a single getByRole().
One thing that comes to mind is using exact: true for that getByRole(). After all, one of the names is "Block Inserter" and the other is "Close Block Inserter", so with an exact match, you should be able to select only one of the buttons. I think that's what @SantosGuillamot proposed above, and I don't see why it wouldn't work.
|
After |
@tyxla as you can see, the tests fail on GitHub with |
Yes, I believe when the updated performance test suite runs against Perhaps @youknowriad will have alternative ideas. |
@tyxla I'm not sure I understand how that would work. Removing the performance test changes from this PR wil make it fail. Am I missing something? |
|
@afercia sorry for the confusion. We might need to temporarily support both versions in the performance test spec. That way the spec will work both with See this commit for example of when this was done before. Let me know if that makes sense. |
Thanks for clarifying @tyxla. So you suggest to use two locators and |
I definitely don't intend to block the PR. I was just trying to minimize the impact on performance tests which @t-hamano expressed concerns about. Feel free to go either way, I have no real preference, I was just trying to help. |
afercia
force-pushed
the
fix/toggle-block-inserter-remove-toggle-verb
branch
from
632bbc3 to
b691c1b
Compare
|
Thanks. I reverted the last commit. The approach with two locators and |
|
What I can say about the performance tests is that they are supposed to run on multiple branches at the same time. So yeah, we need to make sure selectors we use can work both for 6.7 and 6.8 (trunk and this PR). Also, from the changes that I see there, It doesn't seem that these changes have any impact on the metrics themselves, they are part of the "preparation" steps of the tests so they won't impact the numbers AFAIK. |
|
Thanks @youknowriad for clarifying an important point. |
There was a problem hiding this comment.
@youknowriad Thanks for the feedback!
If this PR doesn't impact the metrics, I believe the current approach is best.
* Remove the verb Toggle from the Block Inserter button. * Adjust tests. * Try to fix performance test. * Try to fix perf test --------- Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: kristastevens <kristastevens@git.wordpress.org> Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Addresses part of #61483
What?
Note: I'd still like the 'Block Inserter' to be renamed because the current name is too technical and the tool isn't just inseerting blocks any longer. However, this is a first step to make the name better translatable and avoid the verb 'toggle'.
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast