BorderControl: Make height consistent with other controls #40920
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aaronrobertshaw
added
[Type] Enhancement
Contributor
Author
|
@ciampo Do you have any thoughts on better ways to approach setting the height for this control given there should be a shift to I'm not sure how easy it will be to target the inner components/elements if we were to try and style it only from the editor side for now. |
This was referenced May 9, 2022
andrewserong
approved these changes
May 10, 2022
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
Hard-coding this to 30px height in the interim until we get to a standardized 36px sounds good to me! It looks a tiny bit tight when using a dotted border (which is improved when rebasing on trunk where the control has a darker border color), but otherwise I think the consistent height with the other controls makes this a worthwhile change. Thanks for adding the inline comments so it's clear what should be changed in a follow-up when we're ready to go to 36px!
| This PR | This PR rebased on trunk so it has a darker border |
|---|---|
 |
 |
✅ Tested well in Storybook
✅ Looks good to me when adjusting the Group block's border controls in the editor
LGTM!
Contributor
Author
|
Thanks for the review @andrewserong 👍
I agree about this appearing a bit tight and that is despite reducing the size of the color swatch already. A different option was to increase the size of the border-radius control to match the height of the
I'm inclined to agree that the improved height consistency is the better option in the short term. Before looking to merge this, I'll see if I can steal another pixel of space by reducing the gap between the color swatch and its border e.g. the dotted border you mentioned. In addition, I'll see if I can add something like the |
This requires reducing the size of the color swatch indicator. Not sure it's ideal although will be rectified when the default control height increases to 36px.
aaronrobertshaw
force-pushed
the
fix/border-controls-consistent-height
branch
from
1c2ffd3 to
187ffd5
Compare
|
Size Change: +178 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
fix/border-controls-consistent-height
branch
from
187ffd5 to
ec5da37
Compare
Contributor
Author
|
@andrewserong I've reworked the approach in this PR so the control can opt into the future default height of It should also make it pretty clear what code should be removed when the If you have the time for another review, I'd like to get your thoughts on this. If you think the extra changes are too much we can always revert the last commit. P.S. This has also been rebased to include the updated border colors. Screen.Recording.2022-05-10.at.4.23.23.pm.mp4 |
andrewserong
approved these changes
May 10, 2022
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
It should also make it pretty clear what code should be removed when the 30px height is to be deprecated. It also lays the groundwork for supporting multiple heights out of the box.
Nice work @aaronrobertshaw! This is still testing well, the smaller size looks better now it's rebased on trunk and with the adjusted spacing, and it's very cool being able to switch that flag in Storybook and see it immediately render correctly with the new height 👍
I think I usually lean toward avoiding unneeded complexity, but in this case, I think it's worth including the additional changes for a couple of reasons:
- The added complexity is pretty straightforward (it's a boolean flag that's clearly documented)
- Adding in the ability to do this now saves someone the trouble further down the track of having to look at the component and work out how best to update the height again, and furthers the efforts in #39397
Also it looks like it follows the conventions @mirka used in #39397, so this LGTM! 🎉
Contributor
The current approach looks good to me. More details on the plans to increase the default size to
I'm not sure I get this point |
ciampo
approved these changes
May 10, 2022
Contributor
There was a problem hiding this comment.
Changes LGTM 🚀
I only have 2 thoughts (not blocking, though):
- In components which have a few sub-components (like
BorderBoxControl), I wonder if using the internal component's context system could be a better way to propagate the__next36pxDefaultSizeprop instead of "forwarding" it explicitly as a prop on each component - In the styles, I see a lot of hardcoded, arbitrary numbers — I wonder if it made sense to use the
spaceutility more often and/or if it made sense to show the logic behind these numbers (e.g.:const targetHeight = 36; const borderWidth = 1; const vertcalPadding = 2; const innerHeight = targetHeight - 2 * ( borderWidth + vertcalPadding );
Again, more of a reflection (and definitely not blocking)!
Contributor
Author
|
Thanks for the sanity check and the quick review @ciampo 👍
Appreciate the suggestions as always. I'll look to adopt them in a follow-up to this PR. Given the component height does feed into some other styling tweaks being made to the border control components, I'll merge this one now. |
Contributor
Author
I was questioning whether I should be adding what will become an outdated default height to the component and therefore whether styling the component's height via block editor styles would be better. I didn't think it would really work let alone not be brittle. With the latest approach, the question was a bit obsolete. |
Contributor
|
While reviewing, I noticed the spacing between the border radius' |
Contributor
Author
|
Thanks, it was on my list to clean up. I'll review it shortly. |
15 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related:
What?
Makes
BorderControlheight consistent with other controls by default i.e.30px. Also, updatesBorderBoxControlto match.A new prop (
__next36pxDefaultSize) is introduced to allow opting into the future default of36px.Why?
Border controls within the editor are currently inconsistent.
How?
BorderControlheight based on new__next36pxDefaultSizeprop__next36pxDefaultSizeprop through border control components as requiredTesting Instructions
30pxtall.(Border color & focus styles will be addressed separately)
BorderBoxControland confirm the split border view still looks okBorderControlandBorderBoxControlcomponent pagesnpm run storybook:dev__next36pxDefaultSizecontrol and check component renders okScreenshots or screencast