Add margin-bottom lint rules for BaseControl #64355
Merged
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
mirka
commented
Aug 7, 2024
| <BaseControl className="gallery-image-sizes"> | ||
| <BaseControl | ||
| className="gallery-image-sizes" | ||
| __nextHasNoMarginBottom |
There was a problem hiding this comment.
Testing instructions
Somewhere in this file, set the imageSizeOptions variable as false. In the editor, add a Gallery block with an image, and see the block inspector.
| <BaseControl | ||
| label={ __( 'Width' ) } | ||
| id={ unitControlInputId } | ||
| <VStack |
There was a problem hiding this comment.
Testing instructions
In the editor, add a Search block and see the block inspector.
| Before | After |
|---|---|
 |
 |
This UI probably should be updated to DimensionTool, but I'll just do a quick fix here.
Spacing is the same as a similar pattern in the Latest Posts block.
There was a problem hiding this comment.
Looks good, but it would have been suitable for another PR, perhaps with a design person taking a look 🤔
There was a problem hiding this comment.
You may be right! This is still bad though, just incrementally better. I'm keeping an eye on #52223 to figure out what this UI should be ideally.
|
Size Change: +194 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
49 tasks
# Conflicts: # packages/block-library/src/latest-posts/edit.js
mirka
commented
Aug 9, 2024
Comment on lines
+11
to
+17
| .wp-block-tag-cloud__inspector-settings { | ||
| .components-base-control, | ||
| .components-base-control:last-child { | ||
| // Cancel out extra margins added by block inspector | ||
| margin-bottom: 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
So painful 😭 I'm going to propose a better way to deal with these once the deprecation work is done.
There was a problem hiding this comment.
Indeed. Let's definitely open an issue to keep track of those 👍
| setAttributes( { taxonomy: selectedTaxonomy } ) | ||
| } | ||
| /> | ||
| <BaseControl> |
There was a problem hiding this comment.
Testing instructions
In the editor, add a Tag Cloud block and see the block inspector.
| Before | After |
|---|---|
 |
 |
I removed the unnecessary BaseControl and normalized the uneven spacing.
| @@ -83,6 +83,7 @@ const VideoSettings = ( { setAttributes, attributes } ) => { | |||
| ) } | |||
| /> | |||
| <SelectControl | |||
| __next40pxDefaultSize | |||
There was a problem hiding this comment.
This is the "Preload" dropdown in the Video block inspector controls. (It's named like it's a reused component, but currently only used in the Video block.)
| @@ -219,7 +219,7 @@ function VideoEdit( { | |||
| attributes={ attributes } | |||
| /> | |||
| <MediaUploadCheck> | |||
| <BaseControl className="editor-video-poster-control"> | |||
| <div className="editor-video-poster-control"> | |||
There was a problem hiding this comment.
Testing instructions
In the block editor, add a Video block and see the block inspector.
| Before | After |
|---|---|
 |
 |
Switched the unnecessary BaseControl to a div.
Comment on lines
+104
to
+106
| /** | ||
| * `BaseControl` is a component used to generate labels and help text for components handling user inputs. | ||
| * |
There was a problem hiding this comment.
Moved the JSDocs so they are correctly associated with the exported components. Also made sure the code snippets both have the no margin prop.
| @@ -45,7 +49,7 @@ function SizeControl( props ) { | |||
| }; | |||
|
|
|||
| return ( | |||
| <BaseControl { ...baseControlProps }> | |||
| <BaseControl { ...baseControlProps } __nextHasNoMarginBottom> | |||
There was a problem hiding this comment.
Testing instructions
In Global Styles, go to Typography ▸ Font size presets ▸ (whatever option). Enable the Fluid typography and Custom fluid values toggles.
| Before | After |
|---|---|
 |
 |
I normalized the spacing.
I extracted out a labeling bug as a separate issue: #64406.
Comment on lines
+24
to
+25
| // Do not allow manipulation of margin bottom | ||
| __nextHasNoMarginBottom, |
There was a problem hiding this comment.
No back compat concerns because this component is private.
| @@ -323,6 +323,7 @@ function PushChangesToGlobalStylesControl( { | |||
|
|
|||
| return ( | |||
| <BaseControl | |||
| __nextHasNoMarginBottom | |||
There was a problem hiding this comment.
Testing instructions
In the Site Editor, open a page with a Heading block, or add one. In the block inspector, expanded the Advanced section to see the "Apply globally" control at the bottom.
| @@ -143,6 +143,7 @@ export function CreateTemplatePartModalContents( { | |||
| required | |||
| /> | |||
| <BaseControl | |||
| __nextHasNoMarginBottom | |||
There was a problem hiding this comment.
Testing instructions
In the Site Editor, go to Patterns ▸ Add New Pattern button ▸ Add new template part.
| @@ -92,6 +92,7 @@ function PatternOverridesControls( { | |||
| <> | |||
| <InspectorControls group="advanced"> | |||
| <BaseControl | |||
| __nextHasNoMarginBottom | |||
There was a problem hiding this comment.
Testing instructions
In the editor, add an Image block. From the block toolbar three-dot menu, choose "Create pattern". Click "Edit original". In the block inspector, expanded the Advanced section to see the Enable overrides button at the bottom.
|
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. |
tyxla
approved these changes
Aug 12, 2024
| <BaseControl | ||
| label={ __( 'Width' ) } | ||
| id={ unitControlInputId } | ||
| <VStack |
There was a problem hiding this comment.
Looks good, but it would have been suitable for another PR, perhaps with a design person taking a look 🤔
Comment on lines
+11
to
+17
| .wp-block-tag-cloud__inspector-settings { | ||
| .components-base-control, | ||
| .components-base-control:last-child { | ||
| // Cancel out extra margins added by block inspector | ||
| margin-bottom: 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
Indeed. Let's definitely open an issue to keep track of those 👍
getdave
pushed a commit
that referenced
this pull request
Aug 14, 2024
* Fix in Gallery block * Fix in Latest Posts block * Fix in Search block * Fix in Tag Cloud block * Fix in Video block * Fix in Global Styles Font Size * Fix in Global Styles PushChangesToGlobalStylesControl * Fix in new template part modal * Fix in pattern overrides block inspector * Add lint rule * Update docs Co-authored-by: mirka <[email protected]> Co-authored-by: tyxla <[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.
Part of #38730
What?
Adds eslint rules to prevent new instances of
BaseControlto be introduced in the Gutenberg codebase without the__nextHasNoMarginBottomprop being added.Why?
These lint rules should prevent new violating usages from being added, until we are ready to officially deprecate the margins on the BaseControl-based components all at once.
Testing Instructions
See code comments.