RangeControl: remove margin override and add new opt-in prop #45985
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
|
|
|
Size Change: +3 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
brookewp
added
[Type] Enhancement
49 tasks
brookewp
requested review from
ajitbohra,
ellatrix and
geriux
as code owners
There was a problem hiding this comment.
Nice work @brookewp ✨
All the changes test as advertised for me:
✅ Image: Zoom dropdown
✅ Block Supports: Spacing controls
✅ Avatar: Image size
✅ Columns: Number of columns control
✅ File: Height control
✅ Gallery (Latest): Columns slider
✅ Gallery V1: Columns slider
✅ Latest Comments: Number of comments
✅ Latest Posts: Max words in excerpt, Number of items
✅ Query: Number of items & Columns
✅ Media & Text: Media width
✅ Post Featured Image: Overlay opacity
✅ RSS: Number of items, max words in excerpt, and columns
✅ Site Logo: Image width
✅ Tag Cloud: Number of tags
I found two other uses of the range control that could benefit from the __nextHasNoMarginBottom prop to clean up some margin overrides.
- BorderControl uses a range control and overrides its margin here
- ColorPicker uses a range control with inputs for the various parts of a color value. It overrides the margin here and here.
P.S. While it is a very minor edit, we do touch the QueryControls in the components package so that might need a changelog entry.
brookewp
force-pushed
the
update/rangecontrol-margin
branch
from
eb1b10b to
2b06d5e
Compare
Thank you, @aaronrobertshaw, for testing this and for finding more components that could benefit from this! I realize now my search was too limited which is how I missed these - so I'm really glad for this feedback for my upcoming related PRs! I've added a changelog and removed the overrides for *Edited to note that I've added additional testing steps for changes and screenshots to |
aaronrobertshaw
approved these changes
Nov 30, 2022
There was a problem hiding this comment.
Thanks for the continued iteration on this @brookewp!
Also, I meant to say in my earlier review how great the detailed test instructions for this PR are. They really helped expedite the testing and review process 🙇
I've re-tested everything I checked last time, as well as the BorderControl and ColorPicker components (both in the editor and Storybook). It is all working as advertised for me.
There's only one small issue we should tweak before landing this.
The changelog entry I suggested previously should be in the components package's changelog. My apologies for not being clearer on that one. The main Gutenberg changelog gets updated through the Gutenberg release process.
Maybe something like the following might work for the changelog?
Example diff
diff --git a/changelog.txt b/changelog.txt
index 8e7f596b07..3a8c82ca84 100644
--- a/changelog.txt
+++ b/changelog.txt
@@ -47,7 +47,6 @@ The following contributors merged PRs in this release:
- FocalPointPicker: Update the design of the focal point handle. ([45053](https://github.com/WordPress/gutenberg/pull/45053))
- FontSizePicker: Update hint text to match the design. ([44966](https://github.com/WordPress/gutenberg/pull/44966))
- CheckboxControl: Move icons out of labels. ([45535](https://github.com/WordPress/gutenberg/pull/45535))
-- RangeControl: remove margin overrides and add new opt-in prop. ([45985](https://github.com/WordPress/gutenberg/pull/45985))
#### Block Editor
diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md
index cee4723a7e..c049f17caa 100644
--- a/packages/components/CHANGELOG.md
+++ b/packages/components/CHANGELOG.md
@@ -14,6 +14,7 @@
- `TabPanel`: Add ability to set icon only tab buttons ([#45005](https://github.com/WordPress/gutenberg/pull/45005)).
- `InputControl`, `NumberControl`, `UnitControl`: Add `help` prop for additional description ([#45931](https://github.com/WordPress/gutenberg/pull/45931)).
+- `BorderControl`, `ColorPicker` & `QueryControls`: Replace bottom margin overrides with `__nextHasNoMarginBottom` ([#45985](https://github.com/WordPress/gutenberg/pull/45985)).
### Experimental
I'll approve this one now in anticipation of the changelog tweak, so it's ready to go.
|
@aaronrobertshaw, thanks for taking another look and for the changelog info! I thought I was in the |
mpkelly
pushed a commit
to mpkelly/gutenberg
that referenced
this pull request
Dec 7, 2022
…ss#45985) * RangeControl: Add new opt-in margin bottom prop * Add prop to deprecated blocks * Add prop to gallery v1 block * remove additional margin overrides and add prop to components based on PR feedback * move changelog entry to correct changelog Co-authored-by: Marco Ciampini <[email protected]>
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.
What?
Added new opt-in prop
__nextHasNoMarginBottomfor useages ofRangeControlin the Gutenberg codebase and removed margin override.Why?
Part of this project: #38730
The tl;dr is
BaseControlhas amargin-bottomwhich makes it difficult to reuse and results in inconsistent use.How?
By removing margin overrides in the CSS and adding the prop
__nextHasNoMarginBottom.Testing Instructions
To note, I've included
comment-author-avatarandtext-columnswhich have been deprecated. I also have includedGalleryEditV1, the old version ofGalleryEdit(instructions below on how it can still be found and tested).For
ZoomDropdownFor
SpacingInputControlFor
AvatarInspectorControlsFor
ColumnsEditContainerFor
FileBlockInspectorFor
GalleryEditFor
GalleryEditV1I have tested this myself (thanks @glendaviesnz for the steps) and it's a bit involved to test, but here are the steps if using
wp-envand Docker:gutenberg/.wp-env.json, change the first two lines to:/{DOCKER-CONTAINER}/wp-content/pluginsnpx wp-env run cli wp option set use_balanceTags 1to enableuseBalanceTagsmargin-bottomof 8px which isn't being applied):margin-bottomis 0, spacing stays the same):For
LatestCommentsFor
LatestPostsEdit&QueryControlsFor
MediaTextEditFor
OverlayFor
QueryInspectorControlsFor
RSSEditFor
SiteLogoFor
TagCloudEditIn Storybook:
For
BorderControl:npm run storybook:devFor
ColorPicker: