Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

[WIP] - Portal component experiment #37

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions docs/src/examples/components/Portal/Types/PortalExample.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import React from 'react'

import { Button, Portal } from 'stardust'

class PortalExample extends React.Component {
render() {
return (
<Portal
trigger={
<Button
onClick={() => {
console.log('onClick outer')
}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid creating functions in the render method as it is not a best practice. Treat functions as persisted state and hoist them up to methods. This way, they are the same instance between renders and do not void any equality checks between renders.

>
Open/close portal
</Button>
}
>
<div
style={{
position: 'fixed',
left: '40%',
top: '50%',
padding: '50px',
backgroundColor: 'silver',
}}
>
portal popup
</div>
</Portal>
)
}
}

export default PortalExample
15 changes: 15 additions & 0 deletions docs/src/examples/components/Portal/Types/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import React from 'react'
import ComponentExample from 'docs/src/components/ComponentDoc/ComponentExample'
import ExampleSection from 'docs/src/components/ComponentDoc/ExampleSection'

const Types = () => (
<ExampleSection title="Types">
<ComponentExample
title="Portal"
description="A portal."
examplePath="components/Portal/Types/PortalExample"
/>
</ExampleSection>
)

export default Types
10 changes: 10 additions & 0 deletions docs/src/examples/components/Portal/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import React from 'react'
import Types from './Types'

const PortalExamples = () => (
<div>
<Types />
</div>
)

export default PortalExamples
90 changes: 90 additions & 0 deletions src/components/Portal/Portal.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
import React, { cloneElement } from 'react'
import ReactDOM from 'react-dom'
import PropTypes from 'prop-types'
import { AutoControlledComponent, eventStack, makeDebugger } from '../../lib'

const debug = makeDebugger('portal')

class Portal extends AutoControlledComponent {
static propTypes = {
trigger: PropTypes.node,

open: PropTypes.bool,

defaultOpen: PropTypes.bool,
}

static autoControlledProps = ['open']

handleTriggerClick = () => {
debug('handleTriggerClick()')
this.props.trigger.props.onClick()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will throw if the trigger does not have an onClick prop. Lodash provides a method for safely invoking a path to a function that may or may not exist. We also need to pass the original event back on click:

handleTriggerClick = (e) => {
  _.invoke(this.props.trigger, 'onClick', e)
}

this.setState({ open: !this.state.open })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since open is an auto-controlled, it should be set with this.trySetState(). This method is added by AutoControlledComponent and ensures props and default props are respected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Also marked the second parameter on AutoControlledComponent.trySetState() as optional.

}

handlePortalMouseEnter = () => {
debug('handlePortalMouseEnter()')
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused method?


componentDidMount() {
debug('componentDidMount()', this.state)
if (this.state.open) {
this.createPortal()
}
}

componentDidUpdate() {
debug('componentDidUpdate()', this.state)
if (this.state.open) {
this.createPortal()
}
}

componentWillUnmount() {
debug('componentWillUnmount()')
}

createPortal() {
if (this.state.portalEl) {
return
}
console.log('creating portalEl')
Copy link
Member

@levithomason levithomason Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No log calls. Use debug() instead. You can enable it from the console like so:

localStorage.debug = 'semanticUIReact:portal'
// refresh browser

Note, we should update the debugger namespace in debug.ts to stardust.

const portalEl = document.createElement('div')
document.body.appendChild(portalEl)
eventStack.sub('mouseenter', this.handlePortalMouseEnter, {
target: portalEl,
})
this.setState({
portalEl,
})
}

destroyPortal() {}

// To discuss:
// when to create rootNode? (it is required in render, componentWillMount is deprecated)
// should multiple portals share it? (how would mouseenter/mouseleave on portalEl work then?)
// when to destroy it (it is too early in componentWillUnmount)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain on how we should handle the root node, however, we can use the constructor for a componentWillMount replacement. Could you explain a bit about why componentWillUnmount is too early for removing the mount node?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In componentWillUnmount all the elements which render() created are still in DOM, that means that the mount node still contains the popup.
I was afraid I would confuse React by removing it's elements from DOM directly. But React seems to be fine with that.


render() {
const { trigger } = this.props
debug('render')

if (!trigger) {
return
}

return (
<React.Fragment>
{cloneElement(trigger, {
onClick: this.handleTriggerClick,
})}
{this.state.open &&
this.state.portalEl &&
ReactDOM.createPortal(this.props.children, this.state.portalEl)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's stick with destructuring props and state at the top of our functions. It breaks object references and makes code more readable:

render() {
  const { open, portalEl } = this.state
  // ...
}

</React.Fragment>
)
}
}

export default Portal
1 change: 1 addition & 0 deletions src/components/Portal/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from './Portal'
1 change: 1 addition & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ export { default as Divider } from './components/Divider'
export { default as Layout } from './components/Layout'
export { default as List } from './components/List'
export { ListItem } from './components/List'
export { default as Portal } from './components/Portal'
export { default as Provider } from './components/Provider'
export { default as ProviderConsumer } from './components/Provider/ProviderConsumer'