Skip to content
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.

Sign up for GitHub

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

feat(Portal): Improve rendering #666

Merged
merged 3 commits into from
Oct 17, 2016
Merged

feat(Portal): Improve rendering #666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c49d86
feat(Portal): Improve rendering
jeffcarbs Oct 12, 2016
de4aae3
feat(Portal): Add prepend option
jeffcarbs Oct 13, 2016
bcd490d
Update modal examples, update portal props
jeffcarbs Oct 13, 2016
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
0 .../modules/Modal/Types/ModalBasicExample.js → .../modules/Modal/Types/ModalExampleBasic.js
View file
Open in desktop
File renamed without changes.
0 .../modules/Modal/Types/ModalModalExample.js → .../modules/Modal/Types/ModalExampleModal.js
View file
Open in desktop
File renamed without changes.
52 changes: 52 additions & 0 deletions docs/app/Examples/modules/Modal/Types/ModalExampleMultiple.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import React, { Component } from 'react'
import { Button, Header, Icon, Modal } from 'semantic-ui-react'

class NestedModal extends Component {
state = { open: false }

open = () => this.setState({ open: true })
close = () => this.setState({ open: false })

render() {
const { open } = this.state

return (
<Modal
dimmer={false}
open={open}
onOpen={this.open}
onClose={this.close}
size='small'
trigger={<Button primary icon>Proceed <Icon name='right chevron' /></Button>}
>
<Modal.Header>Modal #2</Modal.Header>
<Modal.Content>
<p>That's everything!</p>
</Modal.Content>
<Modal.Actions>
<Button icon='check' content='All Done' onClick={this.close} />
</Modal.Actions>
</Modal>
)
}
}

const ModalExampleMultiple = () => (
<Modal trigger={<Button>Multiple Modals</Button>}>
<Modal.Header>Modal #1</Modal.Header>
<Modal.Content image>
<div className='image'>
<Icon name='right arrow' />
</div>
<Modal.Description>
<p>We have more to share with you. Follow us along to modal 2</p>
</Modal.Description>
</Modal.Content>
<Modal.Actions>
<NestedModal />
</Modal.Actions>
</Modal>
)

export default ModalExampleMultiple

0 ...ules/Modal/Types/ModalScrollingExample.js → ...ules/Modal/Types/ModalExampleScrolling.js
View file
Open in desktop
File renamed without changes.
10 changes: 5 additions & 5 deletions ...dal/Variations/ModalCloseConfigExample.js → ...dal/Variations/ModalExampleCloseConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,24 @@ import { Button, Icon, Modal } from 'semantic-ui-react'
class ModalCloseConfigExample extends Component {
state = { open: false }

closeConfigShow = (closeOnEscape, closeOnDocumentClick) => () => {
this.setState({ closeOnEscape, closeOnDocumentClick, open: true })
closeConfigShow = (closeOnEscape, closeOnRootNodeClick) => () => {
this.setState({ closeOnEscape, closeOnRootNodeClick, open: true })
}

close = () => this.setState({ open: false })

render() {
const { open, closeOnEscape, closeOnDocumentClick } = this.state
const { open, closeOnEscape, closeOnRootNodeClick } = this.state

return (
<div>
<Button onClick={this.closeConfigShow(false, true)}>No Close on Escape</Button>
<Button onClick={this.closeConfigShow(true, false)}>No Close on Click Outside</Button>
<Button onClick={this.closeConfigShow(true, false)}>No Close on Dimmer Click</Button>

<Modal
open={open}
closeOnEscape={closeOnEscape}
closeOnDocumentClick={closeOnDocumentClick}
closeOnRootNodeClick={closeOnRootNodeClick}
onClose={this.close}
>
<Modal.Header>
Expand Down
12 changes: 10 additions & 2 deletions ...es/Modal/Variations/ModalDimmerExample.js → ...es/Modal/Variations/ModalExampleDimmer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, { Component } from 'react'
import { Button, Header, Icon, Image, Modal } from 'semantic-ui-react'
import { Popup, Button, Header, Icon, Image, Modal } from 'semantic-ui-react'

class ModalDimmerExample extends Component {
state = { open: false }
Expand All @@ -15,7 +15,15 @@ class ModalDimmerExample extends Component {
<Button onClick={this.show(true)}>Default</Button>
<Button onClick={this.show('inverted')}>Inverted</Button>
<Button onClick={this.show('blurring')}>Blurring</Button>
<Button onClick={this.show(false)}>None</Button>
<Popup trigger={<Button onClick={this.show(false)}>None</Button>}>
<Popup.Header>Heads up!</Popup.Header>
<Popup.Content>
By default, a Modal closes when escape is pressed or when the dimmer is
clicked. Setting the dimmer to "None" (dimmer={'{'}false{'}'}) means that there is no
dimmer to click so clicking outside won't close the Modal. To close on
outside click when there's no dimmer, you can pass the "closeOnDocumentClick" prop.
</Popup.Content>
</Popup>

<Modal dimmer={dimmer} open={open} onClose={this.close}>
<Modal.Header>Select a Photo</Modal.Header>
Expand Down
0 ...ules/Modal/Variations/ModalSizeExample.js → ...ules/Modal/Variations/ModalExampleSize.js
View file
Open in desktop
File renamed without changes.
21 changes: 13 additions & 8 deletions docs/app/Examples/modules/Modal/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,43 +10,48 @@ const ModalExamples = () => (
<ComponentExample
title='Modal'
description='A standard modal'
examplePath='modules/Modal/Types/ModalModalExample'
examplePath='modules/Modal/Types/ModalExampleModal'
/>
<ComponentExample
title='Basic'
description='A modal can reduce its complexity'
examplePath='modules/Modal/Types/ModalBasicExample'
examplePath='modules/Modal/Types/ModalExampleBasic'
/>
<ComponentExample
title='Scrolling'
title='Scrolling Modal'
description={[
'When your modal content exceeds the height of the browser the scrollable area will automatically',
'expand to include just enough space for scrolling, without scrolling the page below.',
].join(' ')}
examplePath='modules/Modal/Types/ModalScrollingExample'
examplePath='modules/Modal/Types/ModalExampleScrolling'
>
<Message warning>
<code>&lt;Modal.Content image /&gt;</code> requires an image
with wrapped markup: <code>&lt;Image wrapped /&gt; </code>
</Message>
</ComponentExample>
<ComponentExample
title='Multiple Modals'
description='Multiple modals can be displayed on top of one another'
examplePath='modules/Modal/Types/ModalExampleMultiple'
/>
</ExampleSection>

<ExampleSection title='Variations'>
<ComponentExample
title='Size'
description='A modal can vary in size'
examplePath='modules/Modal/Variations/ModalSizeExample'
examplePath='modules/Modal/Variations/ModalExampleSize'
/>
<ComponentExample
title='Dimmer Variations'
description='A modal can specify dimmer variations'
examplePath='modules/Modal/Variations/ModalDimmerExample'
examplePath='modules/Modal/Variations/ModalExampleDimmer'
/>
<ComponentExample
title='Close Config'
description='Modal can config not to close by escape or click outside'
examplePath='modules/Modal/Variations/ModalCloseConfigExample'
description='Modal can config not to close by escape or dimmer click'
examplePath='modules/Modal/Variations/ModalExampleCloseConfig'
/>
</ExampleSection>
</div>
Expand Down
73 changes: 54 additions & 19 deletions src/addons/Portal/Portal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import ReactDOM from 'react-dom'

import {
AutoControlledComponent as Component,
customPropTypes,
keyboardKey,
makeDebugger,
META,
Expand All @@ -28,8 +29,22 @@ class Portal extends Component {
/** Additional classes. */
className: PropTypes.string,

/**
* Controls whether or not the portal should close on a click on the portal background.
* NOTE: This differs from closeOnDocumentClick:
* - DocumentClick - any click not within the portal
* - RootNodeClick - a click not within the portal but within the portal's wrapper
*/
closeOnRootNodeClick: customPropTypes.every([
customPropTypes.disallow(['closeOnDocumentClick']),
PropTypes.bool,
]),

/** Controls whether or not the portal should close on a click outside. */
closeOnDocumentClick: PropTypes.bool,
closeOnDocumentClick: customPropTypes.every([
customPropTypes.disallow(['closeOnRootNodeClick']),
PropTypes.bool,
]),

/** Controls whether or not the portal should close when escape is pressed is displayed. */
closeOnEscape: PropTypes.bool,
Expand All @@ -53,7 +68,7 @@ class Portal extends Component {
/** Initial value of open. */
defaultOpen: PropTypes.bool,

/** The node where the portal should mount.. */
/** The node where the portal should mount. */
mountNode: PropTypes.any,

/** Milliseconds to wait before closing on mouse leave */
Expand Down Expand Up @@ -86,13 +101,16 @@ class Portal extends Component {
/** Controls whether or not the portal should open when mousing over the trigger. */
openOnTriggerMouseOver: PropTypes.bool,

/** Controls whether the portal should be prepended to the mountNode instead of appended. */
prepend: PropTypes.bool,

/** Element to be rendered in-place where the portal is defined. */
trigger: PropTypes.node,
}

static defaultProps = {
closeOnEscape: true,
closeOnDocumentClick: true,
closeOnEscape: true,
openOnTriggerClick: true,
}

Expand Down Expand Up @@ -136,23 +154,25 @@ class Portal extends Component {
// Document Event Handlers
// ----------------------------------------

closeOnDocumentClick = (e) => {
if (!this.props.closeOnDocumentClick) return
handleDocumentClick = (e) => {
const { closeOnDocumentClick, closeOnRootNodeClick } = this.props

// If event happened in the portal, ignore it
if (this.portal.contains(e.target)) return

debug('closeOnDocumentClick()')
if (closeOnDocumentClick || (closeOnRootNodeClick && this.node.contains(e.target))) {
debug('handleDocumentClick()')

e.stopPropagation()
this.close(e)
e.stopPropagation()
this.close(e)
}
}

closeOnEscape = (e) => {
handleEscape = (e) => {
if (!this.props.closeOnEscape) return
if (keyboardKey.getCode(e) !== keyboardKey.Escape) return

debug('closeOnEscape()')
debug('handleEscape()')

e.preventDefault()
this.close(e)
Expand Down Expand Up @@ -202,14 +222,18 @@ class Portal extends Component {
_.invoke(trigger, 'props.onClick', e)

if (open && closeOnTriggerClick) {
debug('handleTriggerClick() - close')

e.stopPropagation()
this.close(e)
} else if (!open && openOnTriggerClick) {
debug('handleTriggerClick() - open')

e.stopPropagation()
this.open(e)
}

// Prevents closeOnDocumentClick from closing the portal when
// Prevents handleDocumentClick from closing the portal when
// openOnTriggerFocus is set. Focus shifts on mousedown so the portal opens
// before the click finishes so it may actually wind up on the document.
e.nativeEvent.stopImmediatePropagation()
Expand Down Expand Up @@ -298,28 +322,37 @@ class Portal extends Component {

this.mountPortal()

this.node.className = className
this.node.className = className || ''

this.portal = ReactDOM.unstable_renderSubtreeIntoContainer(
ReactDOM.unstable_renderSubtreeIntoContainer(
this,
Children.only(children),
this.node
)

this.portal = this.node.firstElementChild
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this shouldn't be Children.only(children)? If so, it could be assigned to a const at the top of the function and used here and in line 329.

Copy link
Member Author

@jeffcarbs jeffcarbs Oct 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I logged both so you can see:

    console.log('children', Children.only(children))
    console.log('firstElementChild', this.node.firstElementChild)
LOG: 'children', Object{$$typeof: 60103, type: 'p', key: null, ref: null, props: Object{children: 'Hi'}, _owner: null, _store: Object{}}
LOG: 'firstElementChild', <p data-reactroot="">Hi</p>
      ✔ maintains ref to DOM node with host element
LOG: 'children', Object{$$typeof: 60103, type: function Component(props) { ... }, key: null, ref: null, props: Object{}, _owner: null, _store: Object{}}
LOG: 'firstElementChild', <p data-reactroot="">Hi</p>
      ✔ maintains ref to DOM node with React component

In both cases we need that actual DOM node, not a reference to the component or element.


this.portal.addEventListener('mouseleave', this.handlePortalMouseLeave)
this.portal.addEventListener('mouseover', this.handlePortalMouseOver)
}

mountPortal = () => {
if (this.node) return

const { mountNode = document.body } = this.props
debug('mountPortal()')

const { mountNode = document.body, prepend } = this.props

this.node = document.createElement('div')
mountNode.appendChild(this.node)

document.addEventListener('keydown', this.closeOnEscape)
document.addEventListener('click', this.closeOnDocumentClick)
if (prepend) {
mountNode.insertBefore(this.node, mountNode.firstElementChild)
} else {
mountNode.appendChild(this.node)
}

document.addEventListener('click', this.handleDocumentClick)
document.addEventListener('keydown', this.handleEscape)

const { onMount } = this.props
if (onMount) onMount()
Expand All @@ -328,6 +361,8 @@ class Portal extends Component {
unmountPortal = () => {
if (!this.node) return

debug('unmountPortal()')

ReactDOM.unmountComponentAtNode(this.node)
this.node.parentNode.removeChild(this.node)

Expand All @@ -337,8 +372,8 @@ class Portal extends Component {
this.node = null
this.portal = null

document.removeEventListener('keydown', this.closeOnEscape)
document.removeEventListener('click', this.closeOnDocumentClick)
document.removeEventListener('click', this.handleDocumentClick)
document.removeEventListener('keydown', this.handleEscape)

const { onUnmount } = this.props
if (onUnmount) onUnmount()
Expand Down
2 changes: 2 additions & 0 deletions src/modules/Modal/Modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,8 @@ class Modal extends Component {

return (
<Portal
closeOnRootNodeClick
closeOnDocumentClick={false}
Copy link
Member

Choose a reason for hiding this comment

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

Suggest setting defaultProps for this so there is visibility in the docs regarding the behavior. Though, IMO it would seem the default would be to close on escape and on document click. Feedback?

Copy link
Member Author

@jeffcarbs jeffcarbs Oct 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Currently we take unhandled Modal props and pass them to Portal if it handles them. Adding these to defaultProps would mean that they wouldn't get passed to the Portal, since getUnhandledProps includes defaultProps keys in determining what a component handles. We can't just take all props and filter ones that Portal handles because things like className would be double-applied.

Is there any reason why getUnhandledProps can't just use _.keys(Component.propTypes) only? Seems like if there are any autoControlled or defaultProps that a component consumes that aren't defined in the propTypes that's a bigger issue.

{...portalProps}
className={dimmerClasses}
mountNode={this.getMountNode()}
Expand Down
4 changes: 2 additions & 2 deletions test/specs/addons/Confirm-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,9 @@ describe('Confirm', () => {
spy.should.not.have.been.calledOnce()
})

it('is called on body click', () => {
it('is not called on body click', () => {
domEvent.click('body')
spy.should.have.been.calledOnce()
spy.should.not.have.been.calledOnce()
})

it('is called when pressing escape', () => {
Expand Down
Loading