LG-4445: add renderMode and top layer #2482
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
|
stephl3
force-pushed
the
steph/po-render-mode
branch
3 times, most recently
from
17d4fbc
to
deebe1c
Compare
|
Size Change: +1.97 kB (+0.15%) Total Size: 1.36 MB
ℹ️ View Unchanged
|
stephl3
force-pushed
the
steph/po-render-mode
branch
from
deebe1c
to
557fb83
Compare
stephl3
changed the title
| [getTransformStyles(placement, spacing)]: | ||
| state === 'exiting' || state === 'entering', | ||
| [closedStyles]: state === 'exiting' || state === 'exited', | ||
| [openStyles]: state === 'entering' || state === 'entered', | ||
| [css` | ||
| z-index: ${popoverZIndex}; |
There was a problem hiding this comment.
should z-index have a default?
There was a problem hiding this comment.
no, this won't get applied when the popoverZIndex is undefined which is possible in the case when renderMode="top-layer"
There was a problem hiding this comment.
@shaneeza What was the initial reasoning for having PopoverContext live in leafygreen-provider instead of within the popover package?
There was a problem hiding this comment.
fwiw, I did try moving PopoverContext into the popover package while I was consolidating PortalContext and PopoverContext. Since the consolidated PopoverProvider is included in the global LG provider and the @leafygreen-ui/leafygreen-provider, we can't move PopoverContext into popover without introducing a circular dependency
Comment on lines
18
to
29
| export const DismissMode = { | ||
| Auto: 'auto', | ||
| Manual: 'manual', | ||
| } as const; | ||
| export type DismissMode = (typeof DismissMode)[keyof typeof DismissMode]; | ||
|
|
||
| /** Local implementation of web-native `ToggleEvent` until we use typescript v5 */ | ||
| export interface ToggleEvent extends Event { | ||
| type: 'toggle'; | ||
| newState: 'open' | 'closed'; | ||
| oldState: 'open' | 'closed'; | ||
| } |
There was a problem hiding this comment.
I think these types should all live in the popover package
There was a problem hiding this comment.
Why are they duplicated here?
There was a problem hiding this comment.
I have a comment at the top of the file about this. The types can't be shared/imported because of a circular dependency between @leafygreen-ui/leafygreen-provider and @leafygreen-ui/popover
| export type DismissMode = (typeof DismissMode)[keyof typeof DismissMode]; | ||
|
|
||
| /** Local implementation of web-native `ToggleEvent` until we use typescript v5 */ | ||
| export interface ToggleEvent extends Event { |
There was a problem hiding this comment.
I don't think this extends Event, does it? (i.e. it doesn't have e.target etc. iirc)
There was a problem hiding this comment.
This interface inherits properties from its parent, Event.
source: https://developer.mozilla.org/en-US/docs/Web/API/ToggleEvent#instance_properties
took that to mean that it should but can drop if I'm reading that incorrectly
| */ | ||
| scrollContainer?: null; | ||
| }; | ||
| /** @deprecated - use {@link RenderTopLayerProps} */ |
There was a problem hiding this comment.
Could potentially add more on why we recommend TopLayerProps
There was a problem hiding this comment.
going to think more on how to best surface this context before merging the integration branch
| setIsPopoverOpen, | ||
| } = usePopoverContext(); | ||
| ...restProps | ||
| } = usePopoverContextProps(rest, popoverContext); |
There was a problem hiding this comment.
does this need to be a hook?
There was a problem hiding this comment.
felt cleaner to abstract this than have to worry about all the separate variables in the main Popover component file
There was a problem hiding this comment.
I think perhaps this question is more about whether this has to be a hook vs just being a function? It doesn't appear to use any state or call any hooks itself internally.
| @@ -190,16 +231,24 @@ export const Popover = forwardRef<HTMLDivElement, PopoverComponentProps>( | |||
| )} | |||
There was a problem hiding this comment.
do we still need this placeholder?
There was a problem hiding this comment.
yes we still need this for cases where refEl is undefined
Comment on lines
253
to
254
| {/* We need to put `setContentNode` ref on this inner wrapper because | ||
| placing the ref on the parent will create an infinite loop in some cases |
There was a problem hiding this comment.
do we need to use setContentNode? Can we use a standard useRef RefObject?
There was a problem hiding this comment.
we explicitly need to use a callback ref. MutableRefObject caused some positioning issues
Comment on lines
137
to
+142
| <Instance | ||
| buttonText={undefined} | ||
| className={css` | ||
| background-color: ${color.light.background.primary.default}; | ||
| `} | ||
| dismissMode="manual" |
There was a problem hiding this comment.
I don't remember what the answer was last time: does this need to be inside the Button?
There was a problem hiding this comment.
We don't take a stance on this. It only needs to nested under the reference element if the refEl prop is left undefined
| <Popover | ||
| {...rest} | ||
| active={active} | ||
| renderMode={RenderMode.TopLayer} |
There was a problem hiding this comment.
technically we should probably open up the renderMode as a control in the LiveExample
There was a problem hiding this comment.
This in a weird in-between state, so I will leave that off for now until after I've refactored the popover-consuming components. Currently, renderMode="top-layer" is only respected if usePortal={undefined}. After the refactor work, usePortal will no longer be considered and renderMode can be supported as a control
stephl3
force-pushed
the
steph/po-render-mode
branch
from
5fbe049
to
2a4e5eb
Compare
stephl3
force-pushed
the
steph/po-render-mode
branch
4 times, most recently
from
db057de
to
7338ed0
Compare
tsck
reviewed
Oct 3, 2024
There was a problem hiding this comment.
I'm seeing some behavior that seems strange with Select in Storybook where the popover is rendering like in the corner of the page. Does this have anything to do with this? https://642700aa3e49e32bdbf0b0fc-qztpposnja.chromatic.com/?path=/story/components-select--live-example
| setIsPopoverOpen, | ||
| } = usePopoverContext(); | ||
| ...restProps | ||
| } = usePopoverContextProps(rest, popoverContext); |
There was a problem hiding this comment.
I think perhaps this question is more about whether this has to be a hook vs just being a function? It doesn't appear to use any state or call any hooks itself internally.
stephl3
force-pushed
the
steph/po-render-mode
branch
5 times, most recently
from
df9cb86
to
8da3a91
Compare
stephl3
force-pushed
the
steph/po-render-mode
branch
from
8da3a91
to
bee293b
Compare
stephl3
added a commit
that referenced
this pull request
Oct 10, 2024
* 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
stephl3
added a commit
that referenced
this pull request
Oct 11, 2024
* 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
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
renderMode,dismissMode, andonToggleprops toPopovercomponentusePortalcurrently overridesrenderModeprop but will be dropped in an upcoming PRrenderMode="top-layer"🎟 Jira ticket: LG-4445
✅ Checklist
For bug fixes, new features & breaking changes
yarn changesetand documented my changes🧪 How to test changes
PopoverstoriesCombobox/LiveExampleDatePicker/LiveExampleGuideCue/LiveExampleNumberInput/LiveExampleSearchInput/LiveExampleChat/InputBar/WithDropdown