Border Style Control: Update styling for consistency with border width control #37244
Conversation
This file contains 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
…for consistency with Width control
andrewserong
added
[Type] Bug
7 tasks
|
Size Change: +329 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
ciampo
approved these changes
Dec 9, 2021
There was a problem hiding this comment.
Thank you for the ping, Andrew!
Those changes do fix the alignment issue! I believe that this PR is good to go (after the CHANGELOG entry is removed, as explained in the comment below).
I also have some feedback/question on the BorderStyleControl component — again, definitely something that can/should be tackled separately:
- As also highlighted in ToolsPanel: Standardize input control label margin #36387 (review), the proper fix in my opinion is that all components in
@wordpress/componentsget refactored to be based off the same underlyingBaseControl, which would also imply that they share the same implementation for their label. If that was the case, we would then be able to take that base control and use it also in custom controls (like in this example), and get a perfect label alignment out of the box. - Have you considered using
ToggleGroupControlinstead of usingButtons ? - The component is using some classnames (e.g.
components-border-style-control) that use the same naming convention used by the legacy components in@wordpress/components, and therefore somehow imply that this component is part of that package. As we migrate away from hardcodedcomponent-*classnames in@wordpress/components, the fact that the same naming convention is being used in another package will probably add extra confusion. I also know that this comment would apply to most components defined inblock-editor/src/components.
|
I haven't had a chance to review this yet but can answer one of the questions above now:
The control needs to be able to toggle off a selection. The |
ramonjd
approved these changes
Dec 9, 2021
Thanks for the review @ciampo! And apologies, I was working too quickly here and saw
Agreed! Once we've stabilised some of these individual cases for 5.9 it'd be good for us to do this refactor to tidy up the components across the board 👍 |
andrewserong
deleted the
update/border-style-control-to-have-consistent-margin
branch
andrewserong
added
[Package] Block editor
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Dec 13, 2021
…h control (#37244) * Border Style Control: Update line-height, bottom-margin, and padding for consistency with width control
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.
Description
Raised in #34061 (comment) by @ramonjd and following on from the PR by @aaronrobertshaw to standardise ToolsPanel input margins (#36387), this PR looks at updating the Border Style Control to have consistent margin, padding and line-height as the border width control.
How has this been tested?
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).