-
Notifications
You must be signed in to change notification settings - Fork 55
feat(portal): simple base implementation #144
Conversation
b4d9467
to
afa6847
Compare
Codecov Report
@@ Coverage Diff @@
## master #144 +/- ##
==========================================
+ Coverage 89.23% 90.17% +0.94%
==========================================
Files 50 55 +5
Lines 836 916 +80
Branches 130 142 +12
==========================================
+ Hits 746 826 +80
Misses 86 86
Partials 4 4
Continue to review full report at Codecov.
|
logCount: 0, | ||
} | ||
|
||
public render() { |
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 don't think we should be typing public/private like this in React components. Even though method
and other custom methods are publicly accessible, it is not idiomatic React to actually access them. You should never reach into a React component and call a method.
In other words, if we did type them, it would be more proper to consider all methods as private
but I believe that would cause some other issues.
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 don't agree with this and here are my reasons:
-
The current pattern I see all over the project is to not specify any modifier for class members which means all members are public (class members are public by default in typescript).
This is wrong and is one of the main reasons I'd recommend enforcing modifiers; developers often forget to specify a method is private since no modifier is required and this makes it private. -
render
is a public method as it comes fromReact.Component
orUIComponent
, the classes we usually extend. -
Specifying modifiers makes it clear for developers coming from other environments where
private
is the default class member modifier (such asC#
).
We had this discussion before and since it's occurring again I can create an RFC for enforcing modifiers if you think it's valuable.
@levithomason @kuzhelov @miroslavstastny @mnajdova what do you guys think?
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.
since this is an example I'll remove the modifiers so that we make it jsx
, but the points stand for actual typescript code
@@ -0,0 +1,65 @@ | |||
import * as React from 'react' |
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.
No typescript in examples please.
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.
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.
then these should be jsx
files, not tsx
, right? :) I can do the change for now but it's confusing to see these tslint
errors..
@@ -0,0 +1,48 @@ | |||
import * as PropTypes from 'prop-types' | |||
import { invoke } from 'lodash' |
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.
Let's keep convention consistent in the repo, it makes tooling and programmatic updates easier to manage.
There is a babel-plugin-lodash
which we use at SUIR to rewrite the import statements to cherry pick only used methods. We should either implement this plugin, or enforce cherry picking lodash methods, but, we should not mix both patterns without a guarantee of either one.
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.
ok
} | ||
|
||
public render() { | ||
return createPortal(this.props.children, document.body) |
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.
Accessing document
will fail in server side rendering. This should probably be a defaultProp
instead that is safe guarded by a browser check first:
import { isBrowser } from '../../lib'
static defaultProps = {
context: isBrowser() ? document.body : null
}
render() {
const { children, context } = this.props
return createPortal(children, context)
}
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 made the change but I don't really understand it. Server side rendering? Where do we do that?
} | ||
|
||
public componentDidMount() { | ||
invoke(this.props, 'onMount', { ...this.props }) |
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.
Since we're not adding / changing any props here, not sure that a shallow clone is required. We can save on the cycles by just passing this.props
.
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.
made the change as suggested
log: [], | ||
logCount: 0, | ||
open: false, | ||
} |
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.
In effort to minimize lines, this object can be single line:
state = { log: [], logCount: 0, open: false }
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.
done
const { log, logCount, open } = this.state | ||
const content = open ? 'Close Portal' : 'Open Portal' | ||
|
||
const controls = ( |
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.
controls
and portalContent
can also be inlined into the render method to save some lines.
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 my approach makes it more clear how to actually use the components since the controls
and portalContent
are lengthy and users can get confused; that's why I did it like this... Anyway, I made the change as suggested
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.
Thanks for porting this. See individual comments for change request details.
a961ce4
to
83abf5b
Compare
53987e1
to
4f1c8a1
Compare
4f1c8a1
to
c8da43d
Compare
c8da43d
to
d0df7ed
Compare
Portal
This PR will introduce Portal component, a component that allows you to render children outside their parent (for now all portals are rendered at the end of the
body
DOM element); a Portal comes with the following props:onMount?: (props: IPortalProps) => void
: Called when the portal is mounted on the DOMonUnmount?: (props: IPortalProps) => void
: Called when the portal is unmounted from the DOMopen?: boolean
: Controls whether or not the portal is displayedtrigger?: JSX.Element
: Element to be rendered in-place where the portal is definedtriggerRef?: (node: HTMLElement) => void
: Called with a ref to the trigger nodeand the following helper components used by Portal:
PortalInner with props:
onMount?: (props: IPortalProps) => void
: Called when the portal is mounted on the DOMonUnmount?: (props: IPortalProps) => void
: Called when the portal is unmounted from the DOMRef with props:
innerRef?: (ref: HTMLElement) => void
: Called withcomponentDidMount
TODO
Proposal
Basic portal
Renders a portal that is:
trigger
, a component sent to the portal as a prop;trigger
(since it acts like a toggle)generates
and at the end of
body
DOM element:Controlled portal
Renders a portal that is rendered or not depending on the value of
open
propertygenerates
Toggle portal
at the end of
body
DOM element.