Components: Fix label markup for 'FocalPointPicker' #70835
Conversation
Mamaduka
added
[Type] Bug
| // Displays text visually similar to label element. | ||
| // A temp hack for `@wordpress/no-base-control-with-label-without-id`. | ||
| id={ undefined } |
There was a problem hiding this comment.
The BaseControl supports the label prop without an id when it doesn't contain associated form elements as its children.
It appears that the ESLint rule (#14151) is too strict, resulting in incorrect markup. Here's another example:
gutenberg/packages/dataviews/src/dataform-controls/datetime.tsx
Lines 70 to 71 in 7a1f29f
cc @WordPress/gutenberg-components
There was a problem hiding this comment.
Cases like this with multiple fields usually call for fieldset/legend markup, not BaseControl. See for example the BorderControl implementation.
This is a rather common use case, so we're planning to prepare some components to make this easier though.
There was a problem hiding this comment.
Good point. I'll refactor FocalPointPicker to use fieldset markup.
There was a problem hiding this comment.
Here's the refactoring - f915349. Though, it may not be following current best practices of component systems.
|
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. |
|
Flaky tests detected in afadfb4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18864835466
|
Mamaduka
force-pushed
the
fix/focal-point-picker-html-for
branch
from
f915349 to
fc142e1
Compare
Mamaduka
force-pushed
the
fix/focal-point-picker-html-for
branch
from
fc142e1 to
88540f1
Compare
Mamaduka
force-pushed
the
fix/focal-point-picker-html-for
branch
from
88540f1 to
da1f268
Compare
Mamaduka
force-pushed
the
fix/focal-point-picker-html-for
branch
from
da1f268 to
9f73e91
Compare
| <BaseControl | ||
| { ...restProps } | ||
| __nextHasNoMarginBottom={ __nextHasNoMarginBottom } | ||
| __associatedWPComponentName="FocalPointPicker" |
There was a problem hiding this comment.
The deprecation notice for __nextHasNoMarginBottom was being handled through BaseControl, so we'll need to handle it separately. Could you add this back for me?
diff --git a/packages/components/src/focal-point-picker/index.tsx b/packages/components/src/focal-point-picker/index.tsx
index 03d75dfb6c..3485893860 100644
--- a/packages/components/src/focal-point-picker/index.tsx
+++ b/packages/components/src/focal-point-picker/index.tsx
@@ -13,6 +13,7 @@ import {
__experimentalUseDragging as useDragging,
useIsomorphicLayoutEffect,
} from '@wordpress/compose';
+import deprecated from '@wordpress/deprecated';
/**
* Internal dependencies
@@ -109,6 +110,14 @@ export function FocalPointPicker( {
const [ point, setPoint ] = useState( valueProp );
const [ showGridOverlay, setShowGridOverlay ] = useState( false );
+ if ( ! __nextHasNoMarginBottom ) {
+ deprecated( 'Bottom margin styles for wp.components.FocalPointPicker', {
+ since: '6.7',
+ version: '7.0',
+ hint: 'Set the `__nextHasNoMarginBottom` prop to true to start opting into the new styles, which will become the default in a future version.',
+ } );
+ }
+
const { startDrag, endDrag, isDragging } = useDragging( {
onDragStart: ( event ) => {
dragAreaRef.current?.focus();| export const Container = styled( View )` | ||
| border: 0; | ||
| padding: 0; | ||
| margin: 0; |
There was a problem hiding this comment.
I think this is missing a font-size: 13px that was in BaseControl, and it's affecting some spacing 😅
|
Thanks for the review, @mirka! |
What?
PR fixes incorrect
<label forusage inFocalPointPickercomponent. The label has no corresponding form element and should be treated as a visual label only.Notice the error report by the Chrome console.
Testing Instructions
Testing Instructions for Keyboard
Same.
Screenshots or screencast