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

Jump to bottom

BorderBoxControl: use Popover's new anchor prop #43789

Merged
merged 2 commits into from
Sep 5, 2022
Merged

BorderBoxControl: use Popover's new anchor prop #43789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9c8e7c
BorderBoxControl: use new `anchor` prop for `Popover`
ciampo Sep 1, 2022
6eda567
Make sure anchor value is `undefined` instead of `null`
ciampo Sep 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 20 additions & 9 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 @@ -2,7 +2,7 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useRef } from '@wordpress/element';
import { useCallback, useState, useEffect } from '@wordpress/element';
import { useMergeRefs } from '@wordpress/compose';

/**
Expand All @@ -14,7 +14,7 @@ import { Grid } from '../../grid';
import { contextConnect, WordPressComponentProps } from '../../ui/context';
import { useBorderBoxControlSplitControls } from './hook';

import type { SplitControlsProps } from '../types';
import type { SplitControlsProps, PopoverPartialProps } from '../types';

const BorderBoxControlSplitControls = (
props: WordPressComponentProps< SplitControlsProps, 'div' >,
Expand All @@ -36,16 +36,25 @@ const BorderBoxControlSplitControls = (
__next36pxDefaultSize,
...otherProps
} = useBorderBoxControlSplitControls( props );
const containerRef = useRef();
const mergedRef = useMergeRefs( [ containerRef, forwardedRef ] );
const popoverProps = popoverPlacement
? {
const [ popoverProps, setPopoverProps ] = useState< PopoverPartialProps >();
const [ popoverAnchor, setPopoverAnchor ] = useState< Element >();

const containerRef = useCallback( ( node ) => {
setPopoverAnchor( node ?? undefined );
}, [] );

useEffect( () => {
if ( popoverPlacement ) {
Copy link
Contributor Author

@ciampo ciampo Sep 1, 2022 •
edited

Choose a reason for hiding this comment

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

Side note: I'm not in love with the popoverPlacement and popoverOffset props, but these were added as part of #40740 and it's not in the scope of this PR to revisit the APIs of BorderBoxControl

setPopoverProps( {
placement: popoverPlacement,
offset: popoverOffset,
anchorRef: containerRef,
anchor: popoverAnchor,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using anchor instead of anchorRef

__unstableShift: true,
}
: undefined;
} );
} else {
setPopoverProps( undefined );
}
}, [ popoverPlacement, popoverOffset, popoverAnchor ] );

const sharedBorderControlProps = {
colors,
Expand All @@ -58,6 +67,8 @@ const BorderBoxControlSplitControls = (
__next36pxDefaultSize,
};

const mergedRef = useMergeRefs( [ containerRef, forwardedRef ] );

return (
<Grid { ...otherProps } ref={ mergedRef } gap={ 4 }>
<BorderBoxControlVisualizer
Expand Down
30 changes: 21 additions & 9 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 @@ -2,7 +2,7 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useRef } from '@wordpress/element';
import { useCallback, useState, useEffect } from '@wordpress/element';
import { useMergeRefs } from '@wordpress/compose';

/**
Expand All @@ -18,7 +18,7 @@ import { VisuallyHidden } from '../../visually-hidden';
import { contextConnect, WordPressComponentProps } from '../../ui/context';
import { useBorderBoxControl } from './hook';

import type { BorderBoxControlProps } from '../types';
import type { BorderBoxControlProps, PopoverPartialProps } from '../types';
import type { LabelProps } from '../../border-control/types';

const BorderLabel = ( props: LabelProps ) => {
Expand Down Expand Up @@ -62,16 +62,28 @@ const BorderBoxControl = (
__next36pxDefaultSize = false,
...otherProps
} = useBorderBoxControl( props );
const containerRef = useRef();
const mergedRef = useMergeRefs( [ containerRef, forwardedRef ] );
const popoverProps = popoverPlacement
? {

const [ popoverProps, setPopoverProps ] = useState< PopoverPartialProps >();
const [ popoverAnchor, setPopoverAnchor ] = useState< Element >();

const containerRef = useCallback( ( node ) => {
setPopoverAnchor( node ?? undefined );
}, [] );

useEffect( () => {
if ( popoverPlacement ) {
setPopoverProps( {
placement: popoverPlacement,
offset: popoverOffset,
anchorRef: containerRef,
anchor: popoverAnchor,
__unstableShift: true,
}
: undefined;
} );
} else {
setPopoverProps( undefined );
}
}, [ popoverPlacement, popoverOffset, popoverAnchor ] );

const mergedRef = useMergeRefs( [ containerRef, forwardedRef ] );

return (
<View className={ className } { ...otherProps } ref={ mergedRef }>
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/border-box-control/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ Default.args = {
width: '1px',
},
__next36pxDefaultSize: false,
popoverPlacement: 'right-start',
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 added popoverPlacement while testing for the stale anchor value, and decided to keep it for convenience.

But the component's stories definitely need to be rewritten in TypeScript so that they can show all props

Copy link
Contributor

Choose a reason for hiding this comment

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

I have it on my radar to rewrite the border control component stories in TypeScript and update them to follow the latest approach and guidelines. Unfortunately, I haven't had the bandwidth in the lead-up to 6.1 but hope to in the not-too-distant future.

};

const WrapperView = styled.div`
Expand Down
18 changes: 16 additions & 2 deletions packages/components/src/border-box-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import type { Placement, ReferenceType } from '@floating-ui/react-dom';

/**
* Internal dependencies
*/
Expand All @@ -14,6 +19,15 @@ export type AnyBorder = Border | Borders | undefined;
export type BorderProp = keyof Border;
export type BorderSide = keyof Borders;

// TODO: replace with Popover actual props once the component
// gets converted to TypeScript
export type PopoverPartialProps = {
placement?: Placement;
offset?: number;
anchor?: ReferenceType;
__unstableShift?: boolean;
};
Comment on lines +24 to +29
Copy link
Contributor Author

@ciampo ciampo Sep 1, 2022 •
edited

Choose a reason for hiding this comment

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

This should be removed and replaced with the original PopoverProps type once we type the component (hopefully soon!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on it #43823


export type BorderBoxControlProps = ColorProps &
LabelProps & {
/**
Expand All @@ -29,7 +43,7 @@ export type BorderBoxControlProps = ColorProps &
/**
* The position of the color popovers compared to the control wrapper.
*/
popoverPlacement?: string;
popoverPlacement?: Placement;
/**
* The space between the popover and the control wrapper.
*/
Expand Down Expand Up @@ -103,7 +117,7 @@ export type SplitControlsProps = ColorProps & {
/**
* The position of the color popovers compared to the control wrapper.
*/
popoverPlacement?: string;
popoverPlacement?: Placement;
/**
* The space between the popover and the control wrapper.
*/
Expand Down