Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow negative values for margin controls #60347

Merged
merged 16 commits into from
Apr 22, 2024
Merged

Allow negative values for margin controls #60347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
84567a6
allow negative margins on margin controls
MaggieCabrera Apr 1, 2024
b3fa79c
change z-index when block is selected in the editor
MaggieCabrera Apr 1, 2024
5e89477
reduced exagerated z-index
MaggieCabrera Apr 1, 2024
b790667
Add "has-child-selected".
jasmussen Apr 2, 2024
5959a63
added position relative to groups with backgrounds
MaggieCabrera Apr 2, 2024
4d1bba8
target only group blocks
MaggieCabrera Apr 3, 2024
da9ba05
added period to comment
MaggieCabrera Apr 3, 2024
e5f328b
avoid dragging input to negative values
MaggieCabrera Apr 4, 2024
1030a29
fix min values for non margin inputs
MaggieCabrera Apr 4, 2024
3903fe1
change min value on box control component too
MaggieCabrera Apr 9, 2024
1af9164
disable slider when value is out of bounds
MaggieCabrera Apr 9, 2024
21bed04
Revert "disable slider when value is out of bounds"
MaggieCabrera Apr 9, 2024
6c98a0e
disable dragging to the negatives on BoxControl
MaggieCabrera Apr 10, 2024
336a9ce
updated changelog
MaggieCabrera Apr 10, 2024
202506d
only apply z-index if negative margins are present
MaggieCabrera Apr 12, 2024
83d7b95
move the changelog entry to unreleased
scruffian Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/base-styles/_z-index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

$z-layers: (
".block-editor-block-list__block::before": 0,
".block-editor-block-list__block.is-selected": 20,
".block-editor-block-switcher__arrow": 1,
".block-editor-block-list__block {core/image aligned wide or fullwide}": 20,
".block-library-classic__toolbar": 31, // When scrolled to top this toolbar needs to sit over block-editor-block-toolbar
Expand Down
8 changes: 8 additions & 0 deletions packages/block-editor/src/components/block-list/content.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,14 @@ _::-webkit-full-page-media, _:future, :root .has-multi-selection .block-editor-b
user-select: none;
}

&.has-negative-margin {
&.is-selected,
&.has-child-selected {
// Bring the selected block forward so we can see it.
z-index: z-index(".block-editor-block-list__block.is-selected");
}
}

.reusable-block-edit-panel * {
z-index: z-index(".block-editor-block-list__block .reusable-block-edit-panel *");
}
Expand Down
11 changes: 11 additions & 0 deletions packages/block-editor/src/components/block-list/use-block-props/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,16 @@ export function useBlockProps( props = {}, { __unstableIsHtml } = {} ) {
);
}

let hasNegativeMargin = false;
if (
wrapperProps?.style?.marginTop?.charAt( 0 ) === '-' ||
wrapperProps?.style?.marginBottom?.charAt( 0 ) === '-' ||
wrapperProps?.style?.marginLeft?.charAt( 0 ) === '-' ||
wrapperProps?.style?.marginRight?.charAt( 0 ) === '-'
) {
hasNegativeMargin = true;
}

return {
tabIndex: blockEditingMode === 'disabled' ? -1 : 0,
...wrapperProps,
Expand Down Expand Up @@ -174,6 +184,7 @@ export function useBlockProps( props = {}, { __unstableIsHtml } = {} ) {
'can-insert-moving-block': canInsertMovingBlock,
'is-editing-disabled': isEditingDisabled,
'has-editable-outline': hasEditableOutline,
'has-negative-margin': hasNegativeMargin,
'is-content-locked-temporarily-editing-as-blocks':
isTemporarilyEditingAsBlocks,
},
Expand Down
2 changes: 2 additions & 0 deletions packages/block-editor/src/components/global-styles/dimensions-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,7 @@ export default function DimensionsPanel( {
<BoxControl
values={ marginValues }
onChange={ setMarginValues }
min={ -Infinity }
label={ __( 'Margin' ) }
sides={ marginSides }
units={ units }
Expand All @@ -574,6 +575,7 @@ export default function DimensionsPanel( {
<SpacingSizesControl
values={ marginValues }
onChange={ setMarginValues }
minimumCustomValue={ -Infinity }
label={ __( 'Margin' ) }
sides={ marginSides }
units={ units }
Expand Down
17 changes: 16 additions & 1 deletion ...block-editor/src/components/spacing-sizes-control/input-controls/spacing-input-control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ export default function SpacingInputControl( {
! isValueSpacingPreset( value )
);

const [ minValue, setMinValue ] = useState( minimumCustomValue );

const previousValue = usePrevious( value );
if (
!! value &&
Expand Down Expand Up @@ -222,13 +224,26 @@ export default function SpacingInputControl( {
}
value={ currentValue }
units={ units }
min={ minimumCustomValue }
min={ minValue }
placeholder={ allPlaceholder }
disableUnits={ isMixed }
label={ ariaLabel }
hideLabelFromVision
className="spacing-sizes-control__custom-value-input"
size={ '__unstable-large' }
onDragStart={ () => {
if ( value?.charAt( 0 ) === '-' ) {
setMinValue( 0 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this lower bound still make sense if minimumCustomValue was > 0? Math.max( 0, minimumCustomValue ) could be more robust in the general case.

}
} }
onDrag={ () => {
if ( value?.charAt( 0 ) === '-' ) {
setMinValue( 0 );
}
} }
Comment on lines +234 to +243
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, I do think that it's better for UX if you only check for the sign on drag start. It would feel broken if you were dragging in the negative range and then it suddenly disallowed sub-zero dragging in the same drag gesture once you slip back a little over zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this because then we would be able to drag from a positive to a negative, which is what we are trying to avoid. It would only jump to 0 if on drag stare we were already in the negatives

Copy link
Member

@mirka mirka Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial logic I suggested on Slack was like this, where the restriction would only kick in if the starting value was non-negative:

						onDragStart={ () => {
-							if ( value?.charAt( 0 ) === '-' ) {
+							if ( value?.charAt( 0 ) !== '-' ) {
								setMinValue( 0 );
							}
						} }

It is also more efficient because you won't need the onDrag handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might have not explained myself. We are aiming to avoid people accidentally dragging to negative numbers, only allowing them if you are typing them directly in the input. Dragging should never let you get into the negatives

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I get that. My suggestion is specifically about what happens when the value was already negative on drag start. I think you should be able to drag freely until the end of the drag gesture if the drag started from an already negative value.

That's just a UX intuition, so feel free to disregard if you feel otherwise!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps one approach could be to check in onDragStart if the number began as negative when dragging starts and then store in state that we're allowing dragging through the range of negative values within onDrag? I.e. if when we start dragging we have a negative value, then allow dragging through any value as normal, and only set min value to 0 in onDrag if the value when we started dragging was non negative. To do that, I think we'd need another state variable for valueWhenStartingToDrag, though perhaps with a better name 😅

In any case, I think it could be worth tweaking in a follow-up, but in manual testing, the current state of this PR feels good to me.

onDragEnd={ () => {
setMinValue( minimumCustomValue );
} }
/>
<RangeControl
onMouseOver={ onMouseOver }
Expand Down
5 changes: 5 additions & 0 deletions packages/block-library/src/group/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,8 @@
// This block has customizable padding, border-box makes that more predictable.
box-sizing: border-box;
}

// We need this so groups with negative margins overlap as expected.
:where(.wp-block-group.wp-block-group-is-layout-constrained) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the doubling-up of class names necessary? Also, should the modifier syntax not be .wp-block-group--is-layout-constrained according to BEM principles?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmh, the specificity is the same with one or two classes, if we use only the second, I am unsure if that class can be anywhere else but a group block but I prefer to keep it safe. And about the BEM principles, I'm not sure if we are following them or not but that is a class added by the layout code and changing it now is probably more disruptive at this point.

position: relative;
}
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Enhancements

- `BoxControl`: Allow negative values for margin controls ([#60347](https://github.com/WordPress/gutenberg/pull/60347)).

### Bug Fix

- `SlotFill`: fixed missing `getServerSnapshot` parameter in slot map ([#60943](https://github.com/WordPress/gutenberg/pull/60943)).
Expand Down
11 changes: 5 additions & 6 deletions packages/components/src/box-control/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,6 @@ import type {
BoxControlValue,
} from './types';

const defaultInputProps = {
min: 0,
};

const noop = () => {};

function useUniqueId( idProp?: string ) {
Expand Down Expand Up @@ -74,7 +70,6 @@ function useUniqueId( idProp?: string ) {
function BoxControl( {
__next40pxDefaultSize = false,
id: idProp,
inputProps = defaultInputProps,
onChange = noop,
label = __( 'Box Control' ),
values: valuesProp,
Expand All @@ -85,6 +80,7 @@ function BoxControl( {
resetValues = DEFAULT_VALUES,
onMouseOver,
onMouseOut,
...inputProps
}: BoxControlProps ) {
const [ values, setValues ] = useControlledState( valuesProp, {
fallback: DEFAULT_VALUES,
Expand Down Expand Up @@ -140,8 +136,11 @@ function BoxControl( {
setIsDirty( false );
};

const min = 'min' in inputProps ? inputProps.min : 0;
const newInputProps = { ...inputProps, min };

const inputControlProps = {
...inputProps,
newInputProps,
onChange: handleOnChange,
onFocus: handleOnFocus,
isLinked,
Expand Down
20 changes: 20 additions & 0 deletions packages/components/src/box-control/input-controls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* WordPress dependencies
*/
import { useInstanceId } from '@wordpress/compose';
import { useState } from '@wordpress/element';
/**
* Internal dependencies
*/
Expand All @@ -28,6 +29,8 @@ export default function BoxInputControls( {
sides,
...props
}: BoxControlInputControlProps ) {
const minimumCustomValue = props.min;
const [ minValue, setMinValue ] = useState( minimumCustomValue );
const generatedId = useInstanceId( BoxInputControls, 'box-control-input' );

const createHandleOnFocus =
Expand Down Expand Up @@ -100,6 +103,9 @@ export default function BoxInputControls( {
? parsedUnit
: selectedUnits[ side ];

const isNegativeValue =
parsedQuantity !== undefined && parsedQuantity < 0;

const inputId = [ generatedId, side ].join( '-' );

return (
Expand All @@ -115,6 +121,7 @@ export default function BoxInputControls( {
value={ [ parsedQuantity, computedUnit ].join(
''
) }
min={ minValue }
onChange={ ( nextValue, extra ) =>
handleOnValueChange(
side,
Expand All @@ -126,6 +133,19 @@ export default function BoxInputControls( {
side
) }
onFocus={ createHandleOnFocus( side ) }
onDragStart={ () => {
if ( isNegativeValue ) {
setMinValue( 0 );
}
} }
onDrag={ () => {
if ( isNegativeValue ) {
setMinValue( 0 );
}
} }
onDragEnd={ () => {
setMinValue( minimumCustomValue );
} }
label={ LABELS[ side ] }
hideLabelFromVision
/>
Expand Down
Loading