-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix escape key close in Modal
#2513
Conversation
|
Size Change: -270 B (-0.02%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
8e9507e
to
92829c4
Compare
ModalPopoverProviderProps, | ||
} from './ModalPopoverContext.types'; | ||
|
||
export const ModalPopoverContext = createContext<ModalPopoverContextType>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense for this to live in packages/modal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It originally lived in @leafygreen-ui/leafygreen-provider
, but after reviewing the consuming packages, I think that is a better home!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheSonOfThomp heads up that in moving this context to the @leafygreen-ui/modal
package, I needed to update some dev dependency relationships with @leafygreen-ui/modal
to prevent circular dependencies
I think that this is the right move for those as well since changing behavior in Code
, Copyable
, and Select
should ensure they still work in a Modal
. They now follow a similar pattern as DatePicker
's InModal
story. In fact, I only identified this issue because of that story
Let me know what you think of these changes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah shoot. I forgot about that. Forcing Popover
to depend on modal
isn't great. Let's sync on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
synced offline; we'll move this back to @leafygreen-ui/leafygreen-provider
to avoid making @leafygreen-ui/modal
an indirect dependency of all packages dependent on @leafygreen-ui/popover
packages/leafygreen-provider/src/ModalPopoverContext/ModalPopoverContext.spec.tsx
Outdated
Show resolved
Hide resolved
packages/leafygreen-provider/src/ModalPopoverContext/ModalPopoverContext.spec.tsx
Outdated
Show resolved
Hide resolved
packages/leafygreen-provider/src/ModalPopoverContext/ModalPopoverContext.types.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a changeset here as well? Also note that bumping a peerDependency
(leafygreen-provider
) require a major change in the consuming component
a8f43ab
to
bd0902a
Compare
Edit: here's the corresponding changeset https://github.com/mongodb/leafygreen-ui/pull/2517/files#diff-973927adc92828cf652f02746c195982b8cdfcdf5603010c8cbb0434e4afe791 Going to put up a PR for changesets today but here's what I have locally for LG provider: '@leafygreen-ui/leafygreen-provider': majorLG-4446: Consolidates popover-related contexts
Internal note: Migration guideOld<PortalContextProvider
popover={{
portalContainer: yourPortalContainer,
scrollContainer: yourScrollContainer,
}}
> New<PopoverProvider
portalContainer={yourPortalContainer}
scrollContainer={yourScrollContainer}
> |
173e82a
to
1bad647
Compare
*/} | ||
<PopoverProvider>{children}</PopoverProvider> | ||
<ModalPopoverProvider>{children}</ModalPopoverProvider> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this one be ModalPopoverProvider
? I'm not sure. Maybe I'm misunderstanding this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I think this is preventing the isPopoverOpen
from propagating up to the enclosing modal
- open modal;
isPopoverOpen=false
- open date picker menu;
isPopoverOpen=true
- open month selector;
isPopoverOpen=true
and a separateisPopoverOpen=true
- close month selector;
isPopoverOpen=true
- close date picker menu;
isPopoverOpen=true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, so this is here in case the date picker is rendered within a modal? And ensures the year/month Select is rendered correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is here in case the date picker is rendered within a modal?
yes!
ensures the year/month Select is rendered correctly?
It will render correctly regardless. Without it, when a year/month select is closed, then isPopoverOpen
is set to false
. Then, if a user hits escape key, the modal instance would close even though the date picker menu is still open
1bad647
to
b25cea8
Compare
/** | ||
* Creates a Modal Popover context. | ||
* Call `useModalPopoverContext` to access the popover state | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some documentation about what this does and why it's needed? It's not immediately clear if this is tracking the Modal popover itself, or popovers within the Modal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the ModalPopoverProvider could use some more documentation, but otherwise lgtm
* Add MigrationContext for global props * Integrate forceUseTopLayer flag into Popover component
✍️ Proposed changes
Fixes a bug introduced after merging PR #2456. Consolidating the
PopoverContext
andPortalContext
made it so that when aPopoverProvider
was added to replace prop-drillingPopover
component props, theisPopoverOpen
was getting reset tofalse
when opened within aModal
. This PR fixes this by breaking out theisPopoverOpen
/setIsPopoverOpen
values into a separateModalPopoverProvider
.Also, moves the
ModalPopoverContext
from@leafygreen-ui/leafygreen-provider
to@leafygreen-ui/modal
and updates additional dev dependency relationships to prevent circular dependencies.🎟 Jira ticket: Name of ticket
✅ Checklist
For bug fixes, new features & breaking changes
yarn changeset
and documented my changesFor new components
🧪 How to test changes
DatePicker/DatePicker/InModal
story