Popover: update positionToPlacement types
#54101
Merged
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
brookewp
added
[Type] Enhancement
|
@mirka - does it make sense to import |
chad1008
reviewed
Sep 1, 2023
ciampo
approved these changes
Sep 4, 2023
There was a problem hiding this comment.
LGTM 🚀
My initial reasoning for using PopoverProps[ 'placement' ] was to avoid referencing floating-ui directly, but I guess it doesn't make much of a difference, and it actually caused issues once the new overlay custom placement was added to Popover.
Could you add a CHANGELOG entry too?
Good to merge once CI passes (rebasing/mergin trunk may help)
Thank you for working on this!
brookewp
force-pushed
the
update/popover-types
branch
from
40966cd
to
3c01a84
Compare
brookewp
added a commit
that referenced
this pull request
Sep 6, 2023
ciampo
added a commit
that referenced
this pull request
Sep 7, 2023
* Add ariakit dependency * Initial creation of component * Add shortcut from legacy tooltip * Replace initial tests with legacy tooltip tests * Add anchor styles for tooltip placement * Ensure tooltip renders only if one child is passed * Replace query with tooltip role and add user actions to ensure tests complete * Remove forwarding refs to match legacy tooltip * Add legacy positioning * Remove unnecessary code and add describedby id * Replace cloneElement with Radix Slot * Replace emotion with sass * Update tests and types * Updated ariakit package and update-related props and types * Remove radix-ui/react-slot related package changes * Add arrow to fix floating-ui errors * Replace zero value in temp fix to avoid NaN errors * resolve test failures after package update * Match legacy styles * Hide tooltip onBlur to match legacy behavior * Revert "resolve test failures after package update" This reverts commit 9182f1e. * Prevent leaking by ensuring tooltip is hidden after each test * Remove ‘overlay’ option to match legacy * hide on anchor interact to match legacy * Temporarily replace placement with position for rollout * Update story after storybook upgrade * Update descriptions in readme and types * Replace legacy tooltip with ariakit tooltip * Add test with modal for new Esc feature * Error for multiple children but continue to render anchor without tooltip * Update snapshots * Update assertions in failing tests * Replace waitFor with findBy * Replace workaround with utility function * Clean up variables * Update package-lock * Add cleanup to test where toBePositioned was used previously * Update Changelog * Update event handling * Fix failing test * Add cleanup for other components testing tooltip * Remove Arrow workaround after floating-ui/core update * Refine test query to avoid conflict * Update tests based on feedback * remove unnecessary type and variable * Simplify by removing unnecessary logic and adding default value to position prop * Add timeout to test for custom delay * Ensure anchor shows when Icon or multiple children * Update types and README * Update snapshots * Remove type ignore after update in #54101 * Fix link control tests by querying explicitly for the spinner --------- Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
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?
This change is based on a diff suggested by @mirka after reviewing upcoming changes to Tooltip in #48440.
Last year, a utility function
positionToPlacementwas created to assist in converting values for the legacypositionprop to the newerplacementprop. #44377The
positionprop supportedoverlay, butplacementdoes not. However, @ciampo shared thatpositionToPlacementdoesn't returnoverlayand proposed a similar diff as well.Why?
The
overlaytype frompositionisn't part ofplacementfromfloating-ui, so we see TS errors in the aforementioned Tooltip. With more upcoming migrations, this may cause further TS errors in other components. Sinceoverlayisn't returned, we can remove it.How?
Replace
PopoverProps['placement']type withPlacementfromfloating-uito remove typeoverlay.Testing Instructions
CI checks pass