Refactor popover to use separate flip and resize props instead of forcePosition #43546
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
talldan
added
[Feature] UI Components
67 tasks
|
Size Change: +63 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Aug 24, 2022
There was a problem hiding this comment.
Here's a few observations that came to mind when thinking about the APIs of Popover:
New props' default value
IMO, it would make more sense for those two props to have a default value of false:
- it aligns them with
__unstableShift, which also has a default value offalse - it makes sense since those are "special" behaviors that the popover would not have by default
The only issue with that is that until now __unstableForcePosition had a default value of false, which meant that the flip and resize behaviours were on by default.
Avoiding breaking changes
Furthermore, we can't remove support abruptly for the __unstableForcePosition prop — we'd have to keep supporting it and, in the meantime, mark it as deprecated.
Stabilizing props
Finally, i'd like to look into promoting the flip / resize / shift-related prop to being stable props (e.g removing prefixes).
A potential approach
With all of the above in mind, I thought of a plan (partially inspired by the way we've been handling style deprecations)
Step 1: introduce new props while supporting __unstableForcePosition
First step is to introduce the new flip and resize props (no __unstable prefix) while still supporting the __unstableForcePosition prop. The __unstableForcePosition would be marked as deprecated, and we would also output a warning for when flip and resize are not explicitly defined:
Something like this
diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js
index 19507d61eb..7e5a79e95f 100644
--- a/packages/components/src/popover/index.js
+++ b/packages/components/src/popover/index.js
@@ -5,13 +5,13 @@
import classnames from 'classnames';
import {
useFloating,
- flip,
- shift,
+ flip as flipMiddleware,
+ shift as shiftMiddleware,
autoUpdate,
arrow,
offset as offsetMiddleware,
limitShift,
- size,
+ size as sizeMiddleware,
} from '@floating-ui/react-dom';
// eslint-disable-next-line no-restricted-imports
import { motion, useReducedMotion } from 'framer-motion';
@@ -123,6 +123,46 @@ const MaybeAnimatedWrapper = forwardRef(
const slotNameContext = createContext();
+const useDeprecatedForcePosition = ( {
+ __unstableForcePosition,
+ flip,
+ resize,
+} ) => {
+ if ( typeof __unstableForcePosition !== 'undefined' ) {
+ deprecated( '__unstableForcePosition prop in Popover component', {
+ since: '6.1',
+ version: '6.3',
+ alternative: '`flip` and `resize` props',
+ } );
+ }
+
+ if ( typeof flip === 'undefined' ) {
+ deprecated( 'flip prop in Popover component', {
+ since: '6.1',
+ version: '6.3',
+ hint: "The prop's default value will be soon changed to be `false`. If you want the Popover to retain the flipping behavior, set the `flip` property to `true`.",
+ } );
+ }
+ if ( typeof resize === 'undefined' ) {
+ deprecated( 'resize prop in Popover component', {
+ since: '6.1',
+ version: '6.3',
+ hint: "The prop's default value will be soon changed to be `false`. If you want the Popover to retain the resizing behavior, set the `resize` property to `true`.",
+ } );
+ }
+
+ // `__unstableForcePosition` used to be `false` by default. Keeping it
+ // `undefined` by default allows us to detect if it is passed to the component
+ const legacyShouldForcePosition = __unstableForcePosition ?? false;
+
+ // When `__unstableForcePosition` was set to `true`, the `flip` and `resize`
+ // behaviors used to be disabled.
+ return {
+ shouldFlip: flip ?? ! legacyShouldForcePosition,
+ shouldResize: resize ?? ! legacyShouldForcePosition,
+ };
+};
+
const Popover = (
{
range,
@@ -144,8 +184,9 @@ const Popover = (
onFocusOutside,
__unstableSlotName = SLOT_NAME,
__unstableObserveElement,
- __unstableFlip = true,
- __unstableResize = true,
+ flip,
+ resize,
+ __unstableForcePosition,
__unstableShift = false,
...contentProps
},
@@ -158,6 +199,12 @@ const Popover = (
} );
}
+ const { shouldFlip, shouldResize } = useDeprecatedForcePosition( {
+ flip,
+ resize,
+ __unstableForcePosition,
+ } );
+
const arrowRef = useRef( null );
const anchorRefFallback = useRef( null );
@@ -234,9 +281,9 @@ const Popover = (
crossAxis: frameOffsetRef.current[ crossAxis ],
};
} ),
- __unstableFlip ? flip() : undefined,
- __unstableResize
- ? size( {
+ shouldFlip ? flipMiddleware() : undefined,
+ shouldResize
+ ? sizeMiddleware( {
apply( sizeProps ) {
const { availableHeight } = sizeProps;
if ( ! refs.floating.current ) return;
@@ -249,7 +296,7 @@ const Popover = (
} )
: undefined,
__unstableShift
- ? shift( {
+ ? shiftMiddleware( {
crossAxis: true,
limiter: limitShift(),
padding: 1, // Necessary to avoid flickering at the edge of the viewport.We would then go through all usages of Popover in gutenberg, and set explicitly the values of the flip and resize props (ie. both false AND true)
Step 2: Remove support for __unstableForcePosition, switch default value for resize and flip
For the WordPress 6.3 release, we can remove support for __unstableForcePosition entirely, and only rely on flip and resize.
In parallel, we will also be able to set the default value of flip and resize to `false.
We should also consider removing the __unstable prefix from the shift prop (but that can be handled separately from this PR).
What do folks think? cc'ing @mirka to hear her opinion on the strategy outlined above
I agree it'd be nice to have those as
The current policy is that an There do seem to be a lot of plugins using It'd probably also be best to stabilize |
talldan
force-pushed
the
replace/popover-force-position-with-resize-and-flip
branch
from
5f20277
to
37c3297
Compare
ciampo
reviewed
Aug 25, 2022
| __unstableShift = false, | ||
| __unstableForcePosition = false, |
There was a problem hiding this comment.
We should probably remove the default value assignment here, so that the if ( __unstableForcePosition ) check later in the code can detect any instance in which the __unstableForcePosition is passed to Popover.
There was a problem hiding this comment.
👍 I've done that, and update the if statement so that it'll show the deprecation message whenever __unstableForcePosition is specified as a prop.
talldan
force-pushed
the
replace/popover-force-position-with-resize-and-flip
branch
from
f263850
to
f3af613
Compare
…ion message is shown whenever the prop is explicitly specified
talldan
force-pushed
the
replace/popover-force-position-with-resize-and-flip
branch
from
f3af613
to
13d941a
Compare
talldan
deleted the
replace/popover-force-position-with-resize-and-flip
branch
|
Looks like this broke a test in trunk? |
|
Hey @ellatrix , that test was not failing on this PR, and on my local machine the test still succeeds on latest How did you determine that this PR broke it? |
|
I'm looking more into this, and it looks like #43617 may be the PR that introduced that breakage, which probably means that those tests are somewhat incompatible with |
|
Tentative fix to the tests in #43734 |
ciampo
added
the
Needs Dev Note
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.
Dev note
See the dev note in #44195
What?
Closes #43191
Deprecates
__unstableForcePositionreplacing it with newflipandresizeprops.Why?
One of the difficulties with
forcePositionis that it's hard to say what it actually does.flipandresizeare easier to understand, and more closely match floating-ui's terms.This is also needed to fix issue #43541 since there needs to be a way to disable
resizewhile still enablingflip.How?
Splits the prop into two separate props that default to
true.Testing Instructions
This shouldn't cause any differences in the UI, but it's worth testing each affected popover.
Legacy widget form
Block Popover Inbetween
Block Popover
trunk)