BoxControl: Refactor and unify the different sides implementation #67626
Conversation
youknowriad
added
[Feature] UI Components
|
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. |
youknowriad
requested review from
oandregal,
ellatrix,
jorgefilipecosta and
ntsekouras
There was a problem hiding this comment.
Thanks for the great unification work @youknowriad 🙌
Did some testing, and according to the stories, there might be a few regressions. Please take a look at the individual comments.
|
Flaky tests detected in df9490d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12199987455
|
youknowriad
force-pushed
the
update/refactor-box-control
branch
from
5160b8b to
df9490d
Compare
There was a problem hiding this comment.
Here's another edge case that looks broken:
- Open http://localhost:50240/?path=/story/components-boxcontrol--default
- Unlink
- Input different values in each side control
- Choose a different unit (
emfor example) for at least one of the sides - Link again.
On trunk: You'll see Mixed without the unit:
In this branch: You'll see Mixed but with a px unit, even if some of the units are em:
Note that even if I select em unit for all sides, I still see px after linking them back.
|
I think the best path is to consider "mixed units" as "mixed value" and disable the unit like in trunk. It should be done now. |
Co-authored-by: Marin Atanasov <[email protected]>
youknowriad
force-pushed
the
update/refactor-box-control
branch
from
f15066c to
5c8d201
Compare
There was a problem hiding this comment.
Code looks good, and this tests well, with no visible changes. Amazing unification work @youknowriad 🚀
Left a few last nits, feel free to ship afterwards.
Thank you 🙌
|
Thanks for the review Marin |
…rdPress#67626) Co-authored-by: youknowriad <[email protected]> Co-authored-by: tyxla <[email protected]>
Related #67625
What?
In order to implement #67625, I need first to simplify the code of the BoxControl component. It contains three duplicated subcomponents that can be easily merged in one by adding an extra
sideprop.This PR does that.
Testing Instructions
There should be no functional or visual change in this PR. You can test this PR in storybook (the different BoxControl stories) and ensure they look and behave exactly like trunk.