URLPopover: use new placement prop instead of legacy position prop#44391
Merged
URLPopover: use new placement prop instead of legacy position prop#44391
Conversation
67 tasks
|
Size Change: +134 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
22 tasks
ciampo
force-pushed
the
refactor/url-popover-placement-position
branch
from
26b5dd6 to
a679575
Compare
tyxla
reviewed
Oct 31, 2022
ciampo
force-pushed
the
refactor/url-popover-placement-position
branch
from
a679575 to
3c17672
Compare
tyxla
approved these changes
Nov 3, 2022
Member
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
LGTM 👍
Just left a minor nitty question, other than that this is good to go 🚀
Member
There was a problem hiding this comment.
Extreme nit, but with that order of logic, if someone passes both position and placement, the deprecated prop will take precedence. Should we do this only if placement is not specified?
Contributor
Author
There was a problem hiding this comment.
Since I moved the default value from position to placement, the placement prop can't really be undefined in the function's body. Therefore, in order to maintain backwards compat, I decided to implement the current logic.
I could refactor the logic and:
- move the default value assignment of
placementfrom the prop destructuring to later in the function body - make it so that when both
placementandpositionare defined,placementtakes the precedence
Let me know if this sounds like a good idea to you! If so, I'll go ahead and implement it
Member
There was a problem hiding this comment.
What you suggested sounds good! I'll leave it to your judgment whether to implement it before merging.
Contributor
Author
There was a problem hiding this comment.
Pushed e149eef with those changes, let me know if it looks good to you!
ciampo
force-pushed
the
refactor/url-popover-placement-position
branch
from
9d012fc to
efd476f
Compare
ciampo
force-pushed
the
refactor/url-popover-placement-position
branch
from
efd476f to
e149eef
Compare
tyxla
approved these changes
Nov 3, 2022
Member
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
Thanks for the extra comments. Looks great 🚀
ciampo
added
the
has dev note
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What?
Part of #44401
Use the new
placementprop instead of the legacypositionprop to define thePopover's placement in theURLPopovercomponent.Why?
With the recent refactor of the
Popovercomponent tofloating-ui, we're in the process of refactoring all of its usages to the newplacementprop (native tofloating-ui). See #44401 for more detailsHow?
placementprop for theURLPopovercomponentpositionprop as deprecated — the prop, however, is still supported by the componentURLPopovercomponent uses the newplacementprop for thePopovercomponent, also thanks to thepositionToPlacementprop that is being exported from thePopovercomponentSee this conversion table that we're currently using to map
positionvalues toplacementvalues.Testing Instructions
trunkScreenshots or screencast
Kapture.2022-10-28.at.17.24.32.mp4
✍️ Dev Note
With the recent refactor of the
Popovercomponent to use thefloating-uilibrary, theplacementprop has become the recommended way to determine how the component positions with respect to its anchor. While the olderpositionprop is still supported, we're making changes towards removing all of its usages in the Gutenberg repository and deprecating it.As part of these efforts, the
positionprop has been marked as deprecated on the following components:Dropdownfrom the@wordpress/componentspackageURLPopovercomponent from the@wordpress/block-editorpackageConsumers of these components should be using the new
placementprop instead.