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.

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

fix(Modal): fix errors with SSR #2260

Merged
merged 2 commits into from
Oct 29, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
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
8 changes: 4 additions & 4 deletions src/addons/Portal/Portal.js
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ class Portal extends Component {
this.mountPortal()

// Server side rendering
if (!isBrowser) return null
if (!isBrowser()) return null

this.rootNode.className = className || ''

Expand All @@ -372,13 +372,13 @@ class Portal extends Component {
}

mountPortal = () => {
if (!isBrowser || this.rootNode) return
if (!isBrowser() || this.rootNode) return

debug('mountPortal()')

const {
eventPool,
mountNode = isBrowser ? document.body : null,
mountNode = isBrowser() ? document.body : null,
prepend,
} = this.props

Expand All @@ -396,7 +396,7 @@ class Portal extends Component {
}

unmountPortal = () => {
if (!isBrowser || !this.rootNode) return
if (!isBrowser() || !this.rootNode) return

debug('unmountPortal()')
const { eventPool } = this.props
Expand Down
2 changes: 1 addition & 1 deletion src/addons/Responsive/Responsive.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export default class Responsive extends Component {
constructor(...args) {
super(...args)

this.state = { width: isBrowser ? window.innerWidth : 0 }
this.state = { width: isBrowser() ? window.innerWidth : 0 }
}

componentDidMount() {
Expand Down
4 changes: 2 additions & 2 deletions src/behaviors/Visibility/Visibility.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ export default class Visibility extends Component {
}

static defaultProps = {
context: isBrowser ? window : null,
context: isBrowser() ? window : null,
continuous: false,
offset: [0, 0],
once: true,
Expand Down Expand Up @@ -196,7 +196,7 @@ export default class Visibility extends Component {
}

componentDidMount() {
if (!isBrowser) return
if (!isBrowser()) return
const { context, fireOnMount } = this.props

this.pageYOffset = window.pageYOffset
Expand Down
2 changes: 1 addition & 1 deletion src/lib/debug.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import _debug from 'debug'
import isBrowser from './isBrowser'

if (isBrowser && process.env.NODE_ENV !== 'production' && process.env.NODE_ENV !== 'test') {
if (isBrowser() && process.env.NODE_ENV !== 'production' && process.env.NODE_ENV !== 'test') {
// Heads Up!
// /~https://github.com/visionmedia/debug/pull/331
//
Expand Down
4 changes: 2 additions & 2 deletions src/lib/eventStack/eventStack.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class EventStack {
// ------------------------------------

sub = (name, handlers, options = {}) => {
if (!isBrowser) return
if (!isBrowser()) return

const { target = document, pool = 'default' } = options
const eventTarget = this._find(target)
Expand All @@ -43,7 +43,7 @@ class EventStack {
}

unsub = (name, handlers, options = {}) => {
if (!isBrowser) return
if (!isBrowser()) return

const { target = document, pool = 'default' } = options
const eventTarget = this._find(target, false)
Expand Down
9 changes: 8 additions & 1 deletion src/lib/isBrowser.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
import _ from 'lodash'

const hasDocument = typeof document === 'object' && document !== null
const hasWindow = typeof window === 'object' && window !== null && window.self === window

export default hasDocument && hasWindow
// eslint-disable-next-line no-confusing-arrow
const isBrowser = () => !_.isNil(isBrowser.override)
? isBrowser.override
: hasDocument && hasWindow

export default isBrowser
4 changes: 2 additions & 2 deletions src/modules/Dimmer/Dimmer.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,15 @@ export default class Dimmer extends Component {
static Dimmable = DimmerDimmable

handlePortalMount = () => {
if (!isBrowser) return
if (!isBrowser()) return

// Heads up, IE doesn't support second argument in add()
document.body.classList.add('dimmed')
document.body.classList.add('dimmable')
}

handlePortalUnmount = () => {
if (!isBrowser) return
if (!isBrowser()) return

// Heads up, IE doesn't support second argument in add()
document.body.classList.remove('dimmed')
Expand Down
3 changes: 3 additions & 0 deletions src/modules/Modal/Modal.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ export interface ModalProps extends PortalProps {

/** Custom styles. */
style?: React.CSSProperties;

/** Element to be rendered in-place where the portal is defined. */
trigger?: React.ReactNode;
}

interface ModalComponent extends React.ComponentClass<ModalProps> {
Expand Down
14 changes: 10 additions & 4 deletions src/modules/Modal/Modal.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import cx from 'classnames'
import _ from 'lodash'
import PropTypes from 'prop-types'
import React from 'react'
import React, { isValidElement } from 'react'

import {
AutoControlledComponent as Component,
Expand Down Expand Up @@ -128,6 +128,9 @@ class Modal extends Component {
/** Custom styles. */
style: PropTypes.object,

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

/**
* NOTE: Any unhandled props that are defined in Portal are passed-through
* to the wrapping Portal.
Expand Down Expand Up @@ -161,7 +164,7 @@ class Modal extends Component {
}

// Do not access document when server side rendering
getMountNode = () => (isBrowser ? this.props.mountNode || document.body : null)
getMountNode = () => (isBrowser() ? this.props.mountNode || document.body : null)

handleActionsOverrides = predefinedProps => ({
onActionClick: (e, actionProps) => {
Expand Down Expand Up @@ -317,11 +320,13 @@ class Modal extends Component {

render() {
const { open } = this.state
const { closeOnDimmerClick, closeOnDocumentClick, dimmer, eventPool } = this.props
const { closeOnDimmerClick, closeOnDocumentClick, dimmer, eventPool, trigger } = this.props
const mountNode = this.getMountNode()

// Short circuit when server side rendering
if (!isBrowser) return null
if (!isBrowser()) {
return isValidElement(trigger) ? trigger : null
}

const unhandled = getUnhandledProps(Modal, this.props)
const portalPropNames = Portal.handledProps
Expand Down Expand Up @@ -358,6 +363,7 @@ class Modal extends Component {
closeOnDocumentClick={closeOnDocumentClick}
closeOnRootNodeClick={closeOnDimmerClick}
{...portalProps}
trigger={trigger}
className={dimmerClasses}
eventPool={eventPool}
mountNode={mountNode}
Expand Down
2 changes: 1 addition & 1 deletion src/modules/Popup/Popup.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ export default class Popup extends Component {
const style = { position: 'absolute' }

// Do not access window/document when server side rendering
if (!isBrowser) return style
if (!isBrowser()) return style

const { offset } = this.props
const { pageYOffset, pageXOffset } = window
Expand Down
2 changes: 1 addition & 1 deletion src/modules/Search/Search.js
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ export default class Search extends Component {
scrollSelectedItemIntoView = () => {
debug('scrollSelectedItemIntoView()')
// Do not access document when server side rendering
if (!isBrowser) return
if (!isBrowser()) return
const menu = document.querySelector('.ui.search.active.visible .results.visible')
const item = menu.querySelector('.result.active')
if (!item) return
Expand Down
6 changes: 3 additions & 3 deletions src/modules/Sticky/Sticky.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export default class Sticky extends Component {
active: true,
bottomOffset: 0,
offset: 0,
scrollContext: isBrowser ? window : null,
scrollContext: isBrowser() ? window : null,
}

static _meta = {
Expand All @@ -93,7 +93,7 @@ export default class Sticky extends Component {
}

componentDidMount() {
if (!isBrowser) return
if (!isBrowser()) return
const { active } = this.props

if (active) {
Expand All @@ -117,7 +117,7 @@ export default class Sticky extends Component {
}

componentWillUnmount() {
if (!isBrowser) return
if (!isBrowser()) return
const { active } = this.props

if (active) this.removeListeners()
Expand Down
37 changes: 27 additions & 10 deletions test/specs/lib/isBrowser-test.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,35 @@
import isBrowser from 'src/lib/isBrowser'

describe('isBrowser', () => {
it('should return true in a browser', () => {
// tests are run in a browser, this should be true
isBrowser.should.be.true()
})
describe('browser', () => {
it('should return true in a browser', () => {
// tests are run in a browser, this should be true
isBrowser().should.be.true()
})

it('should return false when there is no document', () => {
require('imports-loader?document=>undefined!src/lib/isBrowser').default().should.be.false()
require('imports-loader?document=>null!src/lib/isBrowser').default().should.be.false()
})

it('should return false when there is no document', () => {
require('imports-loader?document=>undefined!src/lib/isBrowser').default.should.be.false()
require('imports-loader?document=>null!src/lib/isBrowser').default.should.be.false()
it('should return false when there is no window', () => {
require('imports-loader?window=>undefined!src/lib/isBrowser').default().should.be.false()
require('imports-loader?window=>null!src/lib/isBrowser').default().should.be.false()
})
})

it('should return false when there is no window', () => {
require('imports-loader?window=>undefined!src/lib/isBrowser').default.should.be.false()
require('imports-loader?window=>null!src/lib/isBrowser').default.should.be.false()
describe('server-side', () => {
before(() => {
isBrowser.override = false
})

after(() => {
isBrowser.override = null
})

it('should return override value', () => {
// tests are run in a browser, this should be true
isBrowser().should.be.false()
})
})
})
22 changes: 22 additions & 0 deletions test/specs/modules/Modal/Modal-test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from 'react'
import ReactDOMServer from 'react-dom/server'

import Modal from 'src/modules/Modal/Modal'
import ModalHeader from 'src/modules/Modal/ModalHeader'
Expand All @@ -9,6 +10,7 @@ import Portal from 'src/addons/Portal/Portal'

import { assertNodeContains, assertBodyClasses, assertBodyContains, domEvent, sandbox } from 'test/utils'
import * as common from 'test/specs/commonTests'
import isBrowser from 'src/lib/isBrowser'

// ----------------------------------------
// Wrapper
Expand Down Expand Up @@ -538,4 +540,24 @@ describe('Modal', () => {
})
})
})

describe('server-side', () => {
before(() => {
isBrowser.override = false
})

after(() => {
isBrowser.override = null
})

it('renders empty content when trigger is not a valid component', () => {
const markup = ReactDOMServer.renderToStaticMarkup(<Modal />)
markup.should.equal('')
})

it('renders a valid trigger component', () => {
const markup = ReactDOMServer.renderToStaticMarkup(<Modal trigger={<div id='trigger' />} />)
markup.should.equal('<div id="trigger"></div>')
})
})
})