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

BorderControls: Alternate approach to handling popover positioning #40874

Closed
wants to merge 5 commits into from
Closed

BorderControls: Alternate approach to handling popover positioning #40874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3621019
Fix destructuring of ColorPalette to resolve typing error
aaronrobertshaw May 6, 2022
495b61d
Remove popover positioning CSS for border support
aaronrobertshaw May 6, 2022
a813973
Move handling of popover position/styling to BorderControl
aaronrobertshaw May 6, 2022
3aea82f
Update BorderBoxControl for new popover positioning and responsive st…
aaronrobertshaw May 6, 2022
5a62136
Add new props to story controls
aaronrobertshaw May 6, 2022
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
48 changes: 0 additions & 48 deletions packages/block-editor/src/hooks/border.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,51 +3,3 @@
grid-column: span 1;
}
}

.block-editor__border-box-control__popover,
.block-editor__border-box-control__popover-top,
.block-editor__border-box-control__popover-right,
.block-editor__border-box-control__popover-bottom,
.block-editor__border-box-control__popover-left {
.components-popover__content {
width: 282px;
}
}

$split-border-control-offset: 55px;

@include break-medium() {
.block-editor__border-box-control__popover,
.block-editor__border-box-control__popover-left {
.components-popover__content {
margin-right: #{ $grid-unit-50 + $grid-unit-15 } !important;
}
}

.block-editor__border-box-control__popover-top,
.block-editor__border-box-control__popover-bottom {
.components-popover__content {
margin-right: #{ $grid-unit-50 + $grid-unit-15 + $split-border-control-offset } !important;
}
}

.block-editor__border-box-control__popover-right {
.components-popover__content {
margin-right: #{ $grid-unit-50 + $grid-unit-15 + ( $split-border-control-offset * 2 )} !important;
}
}

.block-editor__border-box-control__popover,
.block-editor__border-box-control__popover-top,
.block-editor__border-box-control__popover-right,
.block-editor__border-box-control__popover-bottom,
.block-editor__border-box-control__popover-left {
&.is-from-top .components-popover__content {
margin-top: #{ -($grid-unit-50 + $grid-unit-15) } !important;
}

&.is-from-bottom .components-popover__content {
margin-bottom: #{ -($grid-unit-50 + $grid-unit-15) } !important;
}
}
}
11 changes: 6 additions & 5 deletions packages/components/src/border-box-control/border-box-control-split-controls/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const BorderBoxControlSplitControls = (
enableAlpha,
enableStyle,
onChange,
popoverClassNames,
rightAlignedClassName,
value,
__experimentalHasMultipleOrigins,
__experimentalIsRenderedInSidebar,
Expand All @@ -50,33 +50,34 @@ const BorderBoxControlSplitControls = (
hideLabelFromVision={ true }
label={ __( 'Top border' ) }
onChange={ ( newBorder ) => onChange( newBorder, 'top' ) }
popoverContentClassName={ popoverClassNames?.top }
value={ value?.top }
__experimentalSide={ 'top' }
{ ...sharedBorderControlProps }
/>
<BorderControl
hideLabelFromVision={ true }
label={ __( 'Left border' ) }
onChange={ ( newBorder ) => onChange( newBorder, 'left' ) }
popoverContentClassName={ popoverClassNames?.left }
value={ value?.left }
__experimentalSide={ 'left' }
{ ...sharedBorderControlProps }
/>
<BorderControl
className={ rightAlignedClassName }
hideLabelFromVision={ true }
label={ __( 'Right border' ) }
onChange={ ( newBorder ) => onChange( newBorder, 'right' ) }
popoverContentClassName={ popoverClassNames?.right }
value={ value?.right }
__experimentalSide={ 'right' }
{ ...sharedBorderControlProps }
/>
<BorderControl
className={ centeredClassName }
hideLabelFromVision={ true }
label={ __( 'Bottom border' ) }
onChange={ ( newBorder ) => onChange( newBorder, 'bottom' ) }
popoverContentClassName={ popoverClassNames?.bottom }
value={ value?.bottom }
__experimentalSide={ 'bottom' }
{ ...sharedBorderControlProps }
/>
</Grid>
Expand Down
11 changes: 10 additions & 1 deletion packages/components/src/border-box-control/border-box-control-split-controls/hook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,14 @@ export function useBorderBoxControlSplitControls(
return cx( styles.CenteredBorderControl, className );
}, [] );

return { ...otherProps, centeredClassName, className: classes };
const rightAlignedClassName = useMemo( () => {
return cx( styles.RightBorderControl, className );
}, [] );
Comment on lines +33 to +35
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found while testing this the alignment of the right border control was out of whack when on tablet or mobile viewports.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make a note to apply this fix even in case this PR doesn't get merged.


return {
...otherProps,
centeredClassName,
className: classes,
rightAlignedClassName,
};
}
23 changes: 2 additions & 21 deletions packages/components/src/border-box-control/border-box-control/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,26 +122,6 @@ _Note: The will be `undefined` if a user clears all borders._

- Required: Yes

### `popoverClassNames`: `Object`

An object defining CSS classnames for all the inner `BorderControl` popover
content.

Example:
```js
{
linked: 'linked-border-popover-content',
top: 'top-border-popover-content',
right: 'right-border-popover-content',
bottom: 'bottom-border-popover-content',
left: 'left-border-popover-content',
}
```

By default, popovers are displayed relative to the button that initiated the popover. By supplying classnames for each individual popover, it is possible to add styling rules to align the popover positions to an unrelated design element, for example, the sidebar inspector in the block editor.

- Required: No

### `value`: `Object`

An object representing the current border configuration.
Expand Down Expand Up @@ -173,6 +153,7 @@ aware of whether the colors prop contains multiple origins.
### `__experimentalIsRenderedInSidebar`: `boolean`

This is passed on to the color related sub-components so they may render more
effectively when used within a sidebar.
effectively when used within a sidebar. If `true` this prop will also adjust
border control popover styles for better sidebar display.

- Required: No
4 changes: 1 addition & 3 deletions packages/components/src/border-box-control/border-box-control/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ const BorderBoxControl = (
linkedValue,
onLinkedChange,
onSplitChange,
popoverClassNames,
splitValue,
toggleLinked,
__experimentalHasMultipleOrigins,
Expand All @@ -77,7 +76,6 @@ const BorderBoxControl = (
placeholder={
hasMixedBorders ? __( 'Mixed' ) : undefined
}
popoverContentClassName={ popoverClassNames?.linked }
shouldSanitizeBorder={ false } // This component will handle that.
value={ linkedValue }
withSlider={ true }
Expand All @@ -88,6 +86,7 @@ const BorderBoxControl = (
__experimentalIsRenderedInSidebar={
__experimentalIsRenderedInSidebar
}
__experimentalSide={ 'all' }
/>
) : (
<BorderBoxControlSplitControls
Expand All @@ -96,7 +95,6 @@ const BorderBoxControl = (
enableAlpha={ enableAlpha }
enableStyle={ enableStyle }
onChange={ onSplitChange }
popoverClassNames={ popoverClassNames }
value={ splitValue }
__experimentalHasMultipleOrigins={
__experimentalHasMultipleOrigins
Expand Down
4 changes: 4 additions & 0 deletions packages/components/src/border-box-control/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,3 +67,7 @@ export const CenteredBorderControl = css`
grid-column: span 2;
margin: 0 auto;
`;

export const RightBorderControl = css`
${ rtl( { marginLeft: 'auto' }, { marginRight: 'auto' } )() }
`;
18 changes: 0 additions & 18 deletions packages/components/src/border-box-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,6 @@ export type AnyBorder = Border | Borders | undefined;
export type BorderProp = keyof Border;
export type BorderSide = keyof Borders;

export type PopoverClassNames = {
linked?: string;
top?: string;
right?: string;
bottom?: string;
left?: string;
};

export type BorderBoxControlProps = ColorProps &
LabelProps & {
/**
Expand All @@ -34,11 +26,6 @@ export type BorderBoxControlProps = ColorProps &
* individual side borders, or `undefined`.
*/
onChange: ( value: AnyBorder ) => void;
/**
* An object defining CSS classnames for all the inner `BorderControl`
* popover content.
*/
popoverClassNames?: PopoverClassNames;
/**
* An object representing the current border configuration.
*
Expand Down Expand Up @@ -84,11 +71,6 @@ export type SplitControlsProps = ColorProps & {
* changed.
*/
onChange: ( value: Border | undefined, side: BorderSide ) => void;
/**
* An object defining CSS classnames for the split side `BorderControl`s'
* popover content.
*/
popoverClassNames?: PopoverClassNames;
/**
* An object representing the current border configuration. It contains
* properties for each side, with each side an object reflecting the border
Expand Down
6 changes: 2 additions & 4 deletions packages/components/src/border-control/border-control-dropdown/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,7 @@ const BorderControlDropdown = (
onReset,
onColorChange,
onStyleChange,
popoverClassName,
popoverContentClassName,
popoverProps,
popoverControlsClassName,
resetButtonClassName,
showDropdownHeader,
Expand Down Expand Up @@ -199,7 +198,6 @@ const BorderControlDropdown = (
</HStack>
) : undefined }
<ColorPalette
className={ popoverContentClassName }
value={ color }
onChange={ onColorChange }
{ ...{ colors, disableCustomColors } }
Expand Down Expand Up @@ -237,7 +235,7 @@ const BorderControlDropdown = (
<Dropdown
renderToggle={ renderToggle }
renderContent={ renderContent }
contentClassName={ popoverClassName }
popoverProps={ popoverProps }
{ ...otherProps }
ref={ forwardedRef }
/>
Expand Down
28 changes: 19 additions & 9 deletions packages/components/src/border-control/border-control-dropdown/hook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { useMemo } from '@wordpress/element';
import * as styles from '../styles';
import { parseQuantityAndUnitFromRawValue } from '../../unit-control/utils';
import { useContextSystem, WordPressComponentProps } from '../../ui/context';
import { useResponsiveValue } from '../../ui/utils/use-responsive-value';
import { useCx } from '../../utils/hooks/use-cx';

import type { DropdownProps } from '../types';
Expand All @@ -20,9 +21,10 @@ export function useBorderControlDropdown(
border,
className,
colors,
contentClassName,
onChange,
previousStyleSelection,
__experimentalIsRenderedInSidebar,
__experimentalSide,
...otherProps
} = useContextSystem( props, 'BorderControlDropdown' );

Expand Down Expand Up @@ -64,18 +66,26 @@ export function useBorderControlDropdown(
return cx( styles.colorIndicatorWrapper( border ) );
}, [ border, cx ] );

// We only want to apply adjusted popover content styles when on desktop.
const responsiveSide = useResponsiveValue( [
undefined,
undefined,
__experimentalSide,
] );
Comment on lines +69 to +74
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 couldn't find the @include break-medium() equivalent using Emotion within the components package. This was my best guess but very happy to switch this out for any better ideas.


const popoverClassName = useMemo( () => {
return cx( styles.borderControlPopover, contentClassName );
}, [ cx, contentClassName ] );
return cx(
styles.borderControlPopover(
__experimentalIsRenderedInSidebar,
responsiveSide
)
);
}, [ cx, __experimentalIsRenderedInSidebar, responsiveSide ] );

const popoverControlsClassName = useMemo( () => {
return cx( styles.borderControlPopoverControls );
}, [ cx ] );

const popoverContentClassName = useMemo( () => {
return cx( styles.borderControlPopoverContent );
}, [ cx ] );

const resetButtonClassName = useMemo( () => {
return cx( styles.resetButton );
}, [ cx ] );
Expand All @@ -90,9 +100,9 @@ export function useBorderControlDropdown(
onColorChange,
onStyleChange,
onReset,
popoverClassName,
popoverContentClassName,
popoverProps: { className: popoverClassName },
popoverControlsClassName,
resetButtonClassName,
__experimentalIsRenderedInSidebar,
};
}
13 changes: 6 additions & 7 deletions packages/components/src/border-control/border-control/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,6 @@ _Note: the value may be `undefined` if a user clears all border properties._

- Required: Yes

### `popoverContentClassName`: `string`

A custom CSS class name to be assigned to the `BorderControl`'s dropdown
popover content.

- Required: No

### `shouldSanitizeBorder`: `boolean`

If opted into, sanitizing the border means that if no width or color have been
Expand Down Expand Up @@ -179,3 +172,9 @@ This is passed on to the color related sub-components so they may render more
effectively when used within a sidebar.

- Required: No

### `__experimentalSide`: 'all' | 'left' | 'top' | 'right' | 'bottom';

Identifies which side if any this border control will be representing.

- Required: No
4 changes: 2 additions & 2 deletions packages/components/src/border-control/border-control/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ const BorderControl = (
onSliderChange,
onWidthChange,
placeholder,
popoverContentClassName,
previousStyleSelection,
showDropdownHeader,
sliderClassName,
Expand All @@ -54,6 +53,7 @@ const BorderControl = (
withSlider,
__experimentalHasMultipleOrigins,
__experimentalIsRenderedInSidebar,
__experimentalSide,
...otherProps
} = useBorderControl( props );

Expand All @@ -68,7 +68,6 @@ const BorderControl = (
<BorderControlDropdown
border={ border }
colors={ colors }
contentClassName={ popoverContentClassName }
disableCustomColors={ disableCustomColors }
enableAlpha={ enableAlpha }
enableStyle={ enableStyle }
Expand All @@ -81,6 +80,7 @@ const BorderControl = (
__experimentalIsRenderedInSidebar={
__experimentalIsRenderedInSidebar
}
__experimentalSide={ __experimentalSide }
/>
<UnitControl
className={ widthControlClassName }
Expand Down
6 changes: 6 additions & 0 deletions packages/components/src/border-control/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ import Popover from '../../popover';
export default {
title: 'Components (Experimental)/BorderControl',
component: BorderControl,
argTypes: {
__experimentalSide: {
options: [ undefined, 'all', 'left', 'top', 'right', 'bottom' ],
},
},
Comment on lines +21 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

Tweaking the __experimentalSide, even with the __experimentalIsRenderedInSidebar flag set to true, doesn't seem to change the appearance of the popover in the default story.

I wonder if we should disable the interactive controls for the story and/or create an ad-hoc story to showcase the "render in sidebar" scenario?

};

// Available border colors.
Expand Down Expand Up @@ -111,6 +116,7 @@ Default.args = {
withSlider: true,
__experimentalIsRenderedInSidebar: false,
__experimentalHasMultipleOrigins: false,
__experimentalSide: undefined,
};

const WrapperView = styled.div`
Expand Down
Loading