Form Input: Don't use flex-direction: row-reverse for checkbox field
#64232
Conversation
t-hamano
added
[Type] Enhancement
|
Size Change: +216 B (+0.01%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
test/integration/fixtures/blocks/core__form-input__deprecated-v2.serialized.html
Outdated
Show resolved
Hide resolved
|
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. |
There was a problem hiding this comment.
Thanks for the PR @t-hamano 👍
After manually testing this one, it is mostly working for me with the exception of the bug you noted with the default block class name being saved as a custom class name. I think that might be related to a misconfiguration of the deprecation supports.
I've left some minor inline comments relating to the new deprecation.
The last thing I noticed is that the original deprecation's fixtures weren't updated. All deprecations have to bring that version of the block up to the latest markup.
This should mean the original v1 deprecation's serialized HTML fixture needed an update. The fact that it didn't and passes might indicate there's another issue here.
|
I think this might be related to #55755 as well... |
|
Thanks for the review!
This was probably the cause 😅 I removed this and the fixture was generated as expected.
This PR only updates markup if the type attribute is |
Thanks for the explanation, it probably means we're missing some fixtures to cover all bases for the block. |
|
I have included blocks with |
There was a problem hiding this comment.
Thanks for the quick work adding the extra fixtures @t-hamano 🚀
I have a couple of suggestions for the new fixtures though.
Like other unit tests, limiting the scope of each test to a single use case is generally beneficial. Other existing blocks limit their fixtures to a single block configuration per fixture e.g. image custom link, media link, captions etc. all getting their own fixture. The different types of form input likely warrant their own independent fixture.
Another benefit in structuring fixtures this way is that it's easier to find relevant fixtures to update when the next deprecation needs to be implemented.
While breaking down the updated v1 fixtures, it might be better to add base fixtures for the block rather than include them under the v1 fixtures. For example: core__form-input__radio, core__form-input__checkbox etc..
This gives a little more flexibility for deprecation fixtures to only target block configurations that are impacted by the relevant deprecation.
What do you think?
|
Sounds like a good idea! I've split it into all available types as variations, but should we remove fixtures that aren't affected by the deprecation? |
|
Flaky tests detected in 8db2101. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10413264875
|
Sorry I wasn't more clear with my suggestion @t-hamano. I was only suggesting that we had a default or base fixture for each of the different form input types. The deprecation fixtures can be specific to the use case or type they are relevant for e.g. the v2 deprecation fixture could only be a checkbox and/or radio as that's the main focus of the change. Some of the input types are just a change of attribute on the I'm probably missing some but maybe we just need base fixtures for:
Does that help clarify things? If not I'm happy to try again 😅 |
|
I reorganized it into the following four fixtures. Are there any fixtures that we don't need? Sorry if I didn't understand 😅
|
It's probably more that I'm not explaining the fixtures I'm proposing very well, without listing every file out individually.
After the latest change, we're still missing the base fixtures for the different input types as outlined in #64232 (comment).
These cover the latest (current) version of the block. The v2 deprecation was only for checkbox and radio inputs, correct? So the deprecation fixtures for v2 probably don't need ones for textarea or other input types that aren't effected. This what I meant by this comment:
We don't have to follow my recommendation if you don't agree with it. If you do, what I think is left is to:
If this isn't making things clearer, I can carve out some time early next week to create a diff or push a commit here if it helps. |
|
I removed the fixtures that I deemed unnecessary. The current fixtures consist of:
If I'm still misunderstanding, feel free to push 🙏 |
|
Thanks @t-hamano
⬆️ This is the bit still missing
Unfortunately, I might not be able to get to this before I'm AFK until the 10th September. |
|
@aaronrobertshaw Thanks for the reply. This PR is not something that needs to be rushed, so I would be happy if you could take a look at it again when you have time. |
All variations of the simple texts input field are added here to match the approach taken in the deprecation fixtures.
aaronrobertshaw
force-pushed
the
form-input/remove-row-reverse
branch
from
e031a00
to
c222f88
Compare
|
@t-hamano I've rebased this and pushed some base fixtures for the various form input types. I've also aligned the default fixture with how you combined all the text, email, number, tel, url inputs in the deprecation fixtures. Given I've contributed the last couple of commits, I'd like to get a fresh set of eyes on this PR for final approval. @andrewserong if you can spare a moment to review this, that would be greatly appreciated 🙏 From my end, I think this should be good to go! |
There was a problem hiding this comment.
Thanks for the ping!
In testing this locally I noticed that the removal of the CSS rule means that any unmigrated content that is already on a site will continue to display in the other order until going in to a post and updating / saving so that the migration is run.
The deprecation runs correctly in the editor:
However prior to running that deprecation in the editor, this is how requests to the site frontend looked with this PR applied:
After adding a paragraph to the post and saving, the update takes effect as expected:
I see this block is still gated behind an experimental feature flag. Does that mean it's safe to make this change because no-one's using it in production sites? Or do we still need to handle markup that's already out in the wild and hasn't been migrated via the deprecation?
Aside from that, code-wise this change LGTM.
|
@aaronrobertshaw Thanks for the update! By looking at your commits, I was able to understand your intentions.
This is likely to occur for all blocks whose save function has been updated. I know some approaches that leave CSS in place for backward compatibility, but I don't know of any clear standards for how much old markup should be supported with CSS. Personally, I feel that for form blocks, it's enough to just ensure that the block doesn't break. If we want to add CSS that takes old markup into account, you can use the following approach: // The old markup, i.e. if there is an input element after the label content, reverse the order as before.
.wp-block-form-input__label:has(.wp-block-form-input__label-content + input[type="checkbox"]) {
flex-direction: row-reverse;
} |
From my perspective, if it's a block that's used in production, it probably either needs CSS that we keep around pretty much forever (as we can't guarantee that old post content on big sites will ever be migrated), or use a server-side approach to dynamically update old markup when the block is rendered. For the Form blocks, since it's behind an experiment, I imagine it's not so important because the blocks aren't being used in production anywhere? I'm not certain of this assumption, though, so I'll just ping @WordPress/gutenberg-core for visibility on that question. |
Yes, we should keep even the CSS for changes in blocks for back compat.
I'd echo this, since it's an experiment.. Also if the css rule is something really simple and would keep the old version as is, I don't see why not add it. |
|
Thank you for the feedback, everyone! I've added some CSS for the old markup. I've also updated the Testing Instructions section so you can test it. |
|
@t-hamano , I was reading the PR description and got confused by the "From" -> "To" code snippet in the "How" section, since the two snippets are identical. Is that a typo? We should update it so that folks visiting this PR can have a better comprehension of its changes :) Thank you!! |
|
@ciampo Thanks for letting me know! That was definitely a typo. I updated it correctly. |
There was a problem hiding this comment.
Thanks for the patience and iteration here @t-hamano. I definitely appreciate this became more involved than expected.
✅ Deprecated version continues to render correctly on the frontend prior to migration
✅ Migration of deprecated version in the editor works as advertised
✅ Migrated block appears correctly on frontend once saved
✅ Deprecation and block fixtures look good and all pass
LGTM 🚢
+1 — my apologies, I meant to come back to this PR and it totally slipped my mind. Thanks for all the follow-up! |
|
Thank you everyone for the reviews 🙇♂️ |
Follow up #63081
What?
This PR dynamically changes the markup to remove
flex-direction: row-reverse;from the Form Input block.Why?
In #63081, a stylelint rule was added to disallow flex-direction reverse values.
In the Form Input block,
flex-direction: row-reverse;is applied to place text after the checkbox element. Currently, it is allowed with a disable comment, but ideally, this rule should not be used.How?
If the
typeattribute ischeckboxorradio, place the label after the input element. This means that the HTML will be changed to the following:Also added block deprecation since we are updating the
savefunction.Note: Updating the HTML for the Form Input block was once attempted in #63081, but was ultimately removed due to the need for block deprecation. The code in this PR is based on this commit.
Testing Instructions
form-input/remove-row-reversebranch.