-
Notifications
You must be signed in to change notification settings - Fork 55
refactor(Portal): allow to use createRef() API in triggerRef #787
Conversation
src/components/Portal/Portal.tsx
Outdated
doesNodeContainClick(this.triggerNode, e) || // event happened in trigger (delegate to trigger handlers) | ||
doesNodeContainClick(this.portalNode, e) // event happened in the portal | ||
!this.portalRef.current || // no portal | ||
doesNodeContainClick(this.triggerRef.current, e) || // event happened in trigger (delegate to trigger handlers) |
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 method doesNodeContainClick
is working with DOM modes only. We should preserve original code of this method and just pass DOM nodes there explicitly - this will make contract of this method to be explicit
src/components/Portal/Portal.tsx
Outdated
this.triggerNode = triggerNode | ||
|
||
_.invoke(this.props, 'triggerRef', triggerNode) | ||
handleRef(this.triggerRef, triggerNode) |
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.
once again this strikes my cognitive process :) I really think that setRef
will be much more intuitive name for this method as, essentially, this is what it does
src/components/Portal/Portal.tsx
Outdated
@@ -57,9 +58,9 @@ export interface PortalProps extends ChildrenComponentProps, ContentComponentPro | |||
/** | |||
* Called with a ref to the trigger node. | |||
* | |||
* @param {JSX.Element} node - Referred node. | |||
* @param {HTMLElement} node - Referred node. |
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 could be removed now
src/components/Portal/Portal.tsx
Outdated
@@ -103,7 +104,7 @@ class Portal extends AutoControlledComponent<ReactPropsStrict<PortalProps>, Port | |||
onUnmount: PropTypes.func, | |||
open: PropTypes.bool, | |||
trigger: PropTypes.node, | |||
triggerRef: PropTypes.func, | |||
triggerRef: PropTypes.oneOfType([PropTypes.func, PropTypes.object]), |
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.
probably, this is a good candidate for custom prop types - would expect this to appear in multiple places
c03fc36
to
b62909a
Compare
The main goal of this PR allow to use
createRef
API intriggerRef
.