LG-4121: consolidate changesets for popover refactor #2517
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
* OverlayContext, useOverlayContext, and OverlayProvider * useOverlay to register, remove, and track isTopMostOverlay * Export overlay context related components and hooks and include provider in LG provider * README and changeset * Fix type and lint * Update registerOverlay logic and use more explicit generic type names
* PopoverContext supports additional props and marks isPopoverOpen and setIsPopoverOpen deprecated * Replace PortalContext with newly extended PopoverContext * Changesets * Add todo in select spec and update todos with jira tix
…ClassName prop (#2457) * Refactor popover styles * Remove redundant contentClassName prop * Clean up Popover positioning logic * Changesets * Reorg code * useIsomorphicLayoutEffect instead of useMemo * Update changesets * Revert changes
* Add @floating-ui/react dependency * Clean up Popover stories * Replace position utils and usePopoverPositioning with useFloating * Remove replaced positioning hook and utils * Fix adjustOnMutation and stories * Changeset * Add useMergeRefs hook * Address feedback
* Update Popover default spacing from 10 to 4 * Changeset * Update README and stories
…2474) * Remove justify fit from @leafygreen-ui/popover * Remove justify fit usage in packages using Popover * Changeset
* Fix usePortal={false} transition and className order
* Replace usePortal with renderMode and add top layer render mode in Popover
* Replace usePortal with renderMode and add top layer render mode in LG Provider
* Refactor types in Popover-consuming packages
* README
* Address feedback
* Replace usePortal with renderMode and refactor Popover-consuming components * Fix tooltip hover delays * Initial feedback * Additional feedback * Fix transition * Fix stories
🦋 Changeset detectedLatest commit: e64e3cd The changes in this PR will be included in the next version bump. This PR includes changesets to release 66 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: 0 B Total Size: 1.39 MB ℹ️ View Unchanged
|
stephl3
commented
Oct 24, 2024
Comment on lines
49
to
60
| * Props passed to the Popover that renders the suggested promps. | ||
| */ | ||
| dropdownProps?: Omit<PopoverRenderModeProps, 'renderMode'>; | ||
| * Props passed to the Popover that renders the suggested prompts. | ||
| */ | ||
| dropdownProps?: Omit< | ||
| PopoverRenderModeProps, | ||
| | 'dismissMode' | ||
| | 'onToggle' | ||
| | 'portalClassName' | ||
| | 'portalContainer' | ||
| | 'portalRef' | ||
| | 'renderMode' | ||
| | 'scrollContainer' | ||
| >; |
There was a problem hiding this comment.
noticed from changesets that this component was missing from the refactor PR
shaneeza
reviewed
Oct 28, 2024
.changeset/combobox.md
Outdated
| '@leafygreen-ui/combobox': major | ||
| --- | ||
|
|
||
| [LG-4121](https://jira.mongodb.org/browse/LG-4121): Replaces `usePortal` prop with `renderMode` prop. `renderMode="inline"` and `renderMode="portal"` are deprecated, and all popover elements should migrate to using the top layer. The old default was `usePortal = true`, and the new default is `renderMode = 'top-layer'`. |
There was a problem hiding this comment.
renderMode="inline"andrenderMode="portal"are deprecated
Aren't we adding these?
There was a problem hiding this comment.
Oh, I see, were inline and portal already available?
There was a problem hiding this comment.
Those replace the old usePortal boolean prop, but they're marked deprecated to discourage further use
There was a problem hiding this comment.
Is it common to mark a new prop value as deprecated?
There was a problem hiding this comment.
I don't think I know enough to comment on how common or not it is
It's the approach that was detailed in this doc: https://docs.google.com/document/d/1RAzYwh2qB9jUNR-ARtM0Oox6J3jmInCK6Awabdl-uNI/edit?tab=t.0#heading=h.5qidc127buw7
shaneeza
reviewed
Oct 28, 2024
shaneeza
reviewed
Oct 28, 2024
shaneeza
reviewed
Oct 28, 2024
shaneeza
reviewed
Oct 28, 2024
shaneeza
reviewed
Oct 28, 2024
shaneeza
reviewed
Oct 28, 2024
shaneeza
reviewed
Oct 28, 2024
| --- | ||
|
|
||
| [LG-4445](https://jira.mongodb.org/browse/LG-4445): Replaces `usePortal` prop with `renderMode` prop with values of `'inline'`, `'portal'`, and `'top-layer'`. `renderMode="inline"` and `renderMode="portal"` are deprecated, and all popover elements should migrate to using the top layer. The old default was `usePortal = true`, and the new default is `renderMode = 'top-layer'`. | ||
| - When `renderMode="top-layer"` or `renderMode` is `undefined`, the popover element will render in the top layer using the [popover API](https://developer.mozilla.org/en-US/docs/Web/API/Popover_API) |
There was a problem hiding this comment.
With the top layer, the popover is still a child of the trigger and can inherit styles. Is that correct? If so, do we mention that anywhere?
There was a problem hiding this comment.
It will inherit styles of its parent, but it won't always be the trigger (e.g. if refEl prop is used). I'm not sure this warrants documenting in our docs since it's a browser behavior
There was a problem hiding this comment.
I think It's at least worth mentioning that usePortal added the popover to the end of the DOM, but the top layer does not.
There was a problem hiding this comment.
There is this bullet:
- When
renderMode="portal", the popover element will portal into a new div appended to the body. Alternatively, it can be portaled into a providedportalContainerelement
Where do you think additional context would best be added?
shaneeza
reviewed
Oct 28, 2024
shaneeza
reviewed
Oct 28, 2024
shaneeza
reviewed
Oct 28, 2024
shaneeza
reviewed
Oct 28, 2024
shaneeza
reviewed
Oct 28, 2024
shaneeza
reviewed
Oct 29, 2024
|
|
||
| Additional changes include: | ||
| - Adds and exports `getPopoverRenderModeProps` util to pick popover props based on given `renderMode` value | ||
| - Deprecates and removes `justify="fit"`. Instead, use `justify="middle"` |
There was a problem hiding this comment.
does the codemod also update the justify prop?
There was a problem hiding this comment.
It does not. I only found 3 usages of it. Do you think it should also be included?
stephl3
force-pushed
the
LG-4121-popover-refactor
branch
from
7fa15ab
to
e02fa10
Compare
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.
✍️ Proposed changes
Consolidates changesets for changes in PR #2520
🎟 Jira ticket: LG-4121