Popover: lock the __experimentalPopoverPositionToPlacement function #47505
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
ciampo
requested review from
talldan,
aaronrobertshaw,
mirka,
tyxla and
andrewserong
ciampo
added
[Package] Components
14 tasks
|
Size Change: +24 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
ciampo
added
the
Needs Dev Note
|
Marking as in need of dev note in case we establish that a prop rename is equivalent to a new prop |
|
I'm thinking that it might be better to avoid stabilizing this function. The cons are that It's a legacy function but will become a stable API indefinitely once released in WordPress core. It will have very limited use as everyone switches over to There seems to only be one use of the function outside of the components package, so other options could be either removing the export and duplicating the function or using the lock/unlock experiments API. |
There was a problem hiding this comment.
The cons are that It's a legacy function but will become a stable API indefinitely once released in WordPress core
This is a good point.
I don't have a strong preference towards either of the options @talldan suggested. Either would make it easier to remove the function in the longer term.
Other than that:
✅ Smoke test in editors didn't reveal any issues
✅ Unit tests pass & no typing errors
tyxla
reviewed
Jan 30, 2023
There was a problem hiding this comment.
I wonder if, in line with what @talldan and @aaronrobertshaw said, it makes sense to deprecate all APIs that are related to the old popover position prop, instead of stabilizing this API? In an ideal world where we're getting rid of deprecated APIs after some period, we'll be able to clean the legacy code up, instead of having it with us forever. Is there a good reason to want to keep maintaining the legacy API?
ciampo
force-pushed
the
stabilize/popover-position-to-placement
branch
from
005ac79
to
35e8dc7
Compare
ciampo
changed the base branch from
trunk
to
update/privatise-__experimentalShowSelectedHint
ciampo
removed
Needs Dev Note
ciampo
changed the title
|
Thanks everyone for the feedback! I think that using Therefore, I went ahead and:
Feel free to give this PR a new round of reviews. I'll merge this PR only after #47229 gets merged. |
noisysocks
reviewed
Feb 2, 2023
packages/components/CHANGELOG.md
Outdated
| @@ -24,6 +24,7 @@ | |||
| - `Button`: Convert to TypeScript ([#46997](https://github.com/WordPress/gutenberg/pull/46997)). | |||
| - `QueryControls`: Convert to TypeScript ([#46721](https://github.com/WordPress/gutenberg/pull/46721)). | |||
| - `Notice`: refactor to TypeScript ([47118](https://github.com/WordPress/gutenberg/pull/47118)). | |||
| - Lock the `__experimentalPopoverPositionToPlacement` function and rename it to `popoverLegacyPositionToPlacement` ([#47505](https://github.com/WordPress/gutenberg/pull/47505)). | |||
There was a problem hiding this comment.
I think we ended up deciding in #47229 that it's nice to keep the __experimental prefix so that can quickly identify which APIs are non-public.
Or maybe we could use a new __private prefix for these cases?
I saw @youknowriad discussing these sorts of things in Slack. What do you think?
When the dust settles on 6.2 we should update backward-compatibility.md with the best practices that we land on.
There was a problem hiding this comment.
Oh, but this is a function! not a prop. OK then I guess that's fine. Maybe. I don't know anymore 😭
There was a problem hiding this comment.
Added the __experimental prefix for good measure (9f3158b). As you say, we can align naming conventions after 6.2 is released
|
Flaky tests detected in 4557ca7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4112506670
|
adamziel
force-pushed
the
update/privatise-__experimentalShowSelectedHint
branch
2 times, most recently
from
d1a795f
to
1709b42
Compare
Base automatically changed from
update/privatise-__experimentalShowSelectedHint
to
trunk
February 6, 2023 10:48
ciampo
force-pushed
the
stabilize/popover-position-to-placement
branch
from
9f3158b
to
0aebe0c
Compare
|
With #47229 merged, I rebased this PR to include the latest changes from This PR should be now ready for a final round of review |
aaronrobertshaw
approved these changes
Feb 7, 2023
There was a problem hiding this comment.
Thanks for iterating on this one @ciampo 👍
It's testing well for me.
✅ Project builds successfully
✅ Storybook examples work as expected
✅ URLPopover is positioned correctly and deprecation warning is output when using position prop
✅ No remaining mentions of __experimentalPopoverPositionToPlacement prop
LGTM! 🚢
tyxla
approved these changes
Feb 7, 2023
ciampo
force-pushed
the
stabilize/popover-position-to-placement
branch
from
0aebe0c
to
4557ca7
Compare
ntsekouras
pushed a commit
that referenced
this pull request
Feb 13, 2023
…47505) * Rename and stabilize __experimentalPopoverPositionToPlacement * Rename internal tests import statement * Revert "Rename internal tests import statement" This reverts commit 4a0602c. * Revert "Rename and stabilize __experimentalPopoverPositionToPlacement" This reverts commit 0d0a7aa. * Use lock/unlock APIs to lock the `positionToPlacement` function * Update CHANGELOG * Add __experimental prefix * Update CHANGELOG
|
Cherry-picked this PR to the wp/6.2 branch. |
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.
What?
Part of #47196
Requires #47229 to be merged first
In an effort to reduce the amount of experimental APIs introduced to the repo, this PR uses the experimental
lock/unlockAPIs to lock the__experimentalPopoverPositionToPlacementfunction from the@wordpress/componentspackage, renaming it to__experimentalPopoverLegacyPositionToPlacement.Why?
I had initially decided to stabilise this APIs, but following the conversations in this PR I later changed the approach and went for
lock/unlock. The whole point here is to prevent the__experimentalPopoverPositionToPlacementfrom being released to core and therefore losing its "experimental" nature.Also, I decided not to introduce any soft deprecations and directly rename the export because it doesn't look like there any usages according to WPDirectory.
How?
@wordpress/components, and instead added the function under theexperimentsobject (which is locked)URLPopover) to use theunlockAPIs to retrieve the same functionTesting Instructions
PopoverStorybook example work as expectedURLPopoveris positioned correctly in the editor (as ontrunk)__experimentalPopoverPositionToPlacementpropDetailed instructions on how to test the `URLPopover` component
positionprop, which should trigger the unlocked functionURLPopoverrenders on top of the block toolbar, and that a warning is printed to the browser's console (sincepositionis deprecated)