Dropdown: deprecate position prop, use popoverProps instead
#46865
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
commented
Jan 3, 2023
| The direction in which the popover should open relative to its parent node. Specify a y- and an x-axis as a space-separated string. Supports `"top"`, `"bottom"` y-axis, and `"left"`, `"center"`, `"right"` x-axis. | ||
|
|
||
| - Required: No | ||
| - Default: `"top center"` |
There was a problem hiding this comment.
I'm not sure that the documented default value for position was correct — the position prop didn't seem to have any default value assigned, and even the prop's default value in the Popover component is different
ciampo
commented
Jan 3, 2023
Comment on lines
+19
to
-21
| options: [ 'firstElement', true, false ], | ||
| control: { | ||
| type: 'radio', | ||
| options: [ 'firstElement', true, false ], |
There was a problem hiding this comment.
This is to fix a warning in Storybook
ciampo
changed the title
Dropdown: deprecate position propDropdown: deprecate position and focusOnMount props
ciampo
changed the title
Dropdown: deprecate position and focusOnMount propsDropdown: deprecate position prop, use popoverProps instead
ciampo
commented
Jan 3, 2023
| position={ position } | ||
| popoverProps={ { position } } |
There was a problem hiding this comment.
The switch to placement should happen in a separate PR (and requires exposing the positionToPlacement utility from Popover too)
|
Size Change: +138 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
ciampo
added
[Type] Code Quality
ciampo
requested review from
mirka,
noisysocks,
talldan,
aaronrobertshaw,
andrewserong,
brookewp and
chad1008
aaronrobertshaw
approved these changes
Jan 10, 2023
There was a problem hiding this comment.
It's good to see the position prop getting the deprecation treatment. Nice work, and thanks for the comprehensive PR description and testing instructions. 🙇
The code changes look good, and everything tested well for me when compared against trunk.
Click for testing results
✅ No typing errors and tests are passing
✅ Storybook example works and controls & docs updated
✅ Cover
✅ Image
✅ Inserter
✅ Tool selector
✅ PostTemplate Template Title
✅ Post Schedule
✅ Post URL
✅ Post Visibility
✅ Document Actions
✅ More Menu
✅ ImportDropdown
PS: I couldn't see where the TableOfContents component was used either
andrewserong
approved these changes
Jan 11, 2023
There was a problem hiding this comment.
+1 thanks for the detailed testing instructions, made reviewing this one a breeze!
Code change looks good, and each of those cases are testing correctly for me, too, so LGTM ✨
PS: I couldn't see where the TableOfContents component was used either
As far as I can tell, the one usage of the component was removed back in this PR: #44788 that switched the document information and outline from being in a dropdown to being in an Outline tab next to the list view. I imagine that component has to stick around since it was exported, even though it's likely not being used anywhere?
|
Thank you both for the review, this was a tedious one 🙏 |
ciampo
added
the
has dev note
GP-Dan-Tovbein
pushed a commit
to greenpeace/planet4-plugin-gutenberg-blocks
that referenced
this pull request
Apr 24, 2023
GP-Dan-Tovbein
pushed a commit
to greenpeace/planet4-plugin-gutenberg-blocks
that referenced
this pull request
May 8, 2023
GP-Dan-Tovbein
pushed a commit
to greenpeace/planet4-plugin-gutenberg-blocks
that referenced
this pull request
May 8, 2023
GP-Dan-Tovbein
pushed a commit
to greenpeace/planet4-plugin-gutenberg-blocks
that referenced
this pull request
Jun 30, 2023
GP-Dan-Tovbein
pushed a commit
to greenpeace/planet4-plugin-gutenberg-blocks
that referenced
this pull request
Aug 23, 2023
GP-Dan-Tovbein
pushed a commit
to greenpeace/planet4-plugin-gutenberg-blocks
that referenced
this pull request
Aug 23, 2023
This was referenced Sep 13, 2023
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.
Closes #46858
What?
Deprecate the
positionprop for theDropdowncomponent:positionprop is about to be deprecated for thePopovercomponent (see Popover: refactor all usages of the legacypositionprop to theplacementprop #44401)popoverProps.placementFollowing this logic, the
focusOnMountprop should also be deprecated in favor of usingpopoverProps.focusOnMountWhy?
Cleaning up code, improving devX
How?
Testing Instructions
Tests pass (this means that no deprecation warnings are printed to console, which means that all usages of
Dropdownhave been migrated away from thepositionprop)Cover block
trunkBlockNavigationDropdown,BlockColorsStyleSelectorNot sure if there's a way to test this component as it's deprecated and seemingly not used anymore.
Image block
trunkInserter
trunkTool selector
trunkPostTemplate Template title
trunktrunkPost schedule
trunkPost URL
trunkPost visibility
trunkDocument actions
trunkTableOfContents
Not sure how to display this component
More menu
trunkImportDropdown
trunk✍️ Dev Note
The dev note in #44391 also covers the changes from this PR.