-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Math format: remove empty format on serialise #73143
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
Conversation
|
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. |
|
Size Change: +11 B (0%) Total Size: 2.41 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 6e88762. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19238273230
|
t-hamano
left a comment
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 have confirmed that this pull request is working correctly.
| const mathInput = page.locator( | ||
| '.block-editor-format-toolbar__math-input input' | ||
| ); |
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.
| const mathInput = page.locator( | |
| '.block-editor-format-toolbar__math-input input' | |
| ); | |
| const mathInput = page.getByRole( 'textbox', { | |
| name: 'LaTeX math syntax', | |
| } ); |
Nit: This selector can be made more accessible.
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, I'll follow-up with a fix so that this definitely makes RC 1.
| await editor.clickBlockToolbarButton( 'More' ); | ||
| await page.getByRole( 'menuitem', { name: 'Math' } ).click(); | ||
|
|
||
| expect( await editor.getBlocks() ).toMatchObject( [ |
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.
| expect( await editor.getBlocks() ).toMatchObject( [ | |
| await expect.poll( editor.getBlocks ).toMatchObject( [ |
It's better to use await expect.poll and async assertions, which benefit from auto-retries. The same applies to similar assertions in this file.
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.
Created PR to fix this in the codebase - #73153.
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 never got this. Why is it not ok to expect this to be correct immediately?
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.
It's more like a safeguard or best practice. Most of the time, it's correct, and the test passes immediately, with no additional overhead. However, when it's not, the auto-retries are helpful to ensure that failure is actually related to not matching the object.
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 a blocker here. We can continue talking in the new PR.
Co-authored-by: ellatrix <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: Mamaduka <[email protected]>
|
I just cherry-picked this PR to the wp/6.9 branch to get it included in the next release: c57fc49 |
|
Thanks for the PR. Tested as described and it works for me. |
Could you elaborate? I'm not quite understanding the issue :) |
What?
When adding inline math in rich text, and you dismiss it or empty it later, it leaves an empty math element.
Why?
This should not happen, we should remove it on serialization. This is similar to how there's no bold until there's content to bold.
How?
When an object in rich text is contenteditable and doesn't have content, don't serialize anything.
Testing Instructions
Add inline math to rich text. Don't write anything in the input. Then check the code editor, there shouldn't be a math element.
Also adds e2e tests for the math format generally.
Testing Instructions for Keyboard
Screenshots or screencast