[Overlay] better lifecycle props for all overlay components #2581
Conversation
- 4 methods passed directly to CSSTransition to leverage their reliable code - remove didOpen, didClose
fix lint & testsPreview: documentation | landing | table |
| @@ -56,6 +52,7 @@ class ContextMenu extends AbstractPureComponent<IContextMenuProps, IContextMenuS | |||
| return ( | |||
| <div className={Classes.CONTEXT_MENU_POPOVER_TARGET} style={this.state.offset}> | |||
| <Popover | |||
| {...this.props} | |||
There was a problem hiding this comment.
this seems like a pretty major change where users could possible pass props to the popover this way, now they can't.
There was a problem hiding this comment.
this component is not even exported, so it think we're safe here. users use either a decorator @ContextMenuTarget or a static method ContextMenu.show() (which looks like this component but isn't).
| /** Lifecycle method invoked when an Overlay has finished transitioning to the closed state. */ | ||
| onClosed?: () => void; | ||
|
|
||
| /** Lifecycle method invoked when an Overlay begins to open. */ |
There was a problem hiding this comment.
i think it might be clearer to say:
Lifecycle method invoked just before an Overlay begins to open.
Because we're dealing with transitions, "when" is ambiguous so we should say "before" or "after" the transition.
Same with onClosing
| @@ -134,31 +134,11 @@ export interface IPopoverProps extends IOverlayableProps, IProps { | |||
| */ | |||
| popoverClassName?: string; | |||
|
|
|||
There was a problem hiding this comment.
this is a major API deprecation. What is the upgrade path for existing code?
There was a problem hiding this comment.
Note: this interface inherits the new lifecycle interface, so this will just be a rename.
improve lifecycle prop docsPreview: documentation | landing | table |
add node to select usagesPreview: documentation | landing | table |
|
@themadcreator can you review my recent commits? made some changes worth looking at. |
Fixes #2402
Changes proposed in this pull request:
IOverlayLifecyclePropsdefinesonOpening,onOpened,onClosing,onClosedonOpeningandonClosedthe content was not in the DOM (before entering or after exiting, respectively)OverlaydidOpen,didCloseprops with those abovePopoverpopoverDid/Will*props with those aboveReviewers should focus on:
could this be an issue? @invliD thoughts?