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

added suport for isomorphic rendering #70

Merged
merged 2 commits into from
Oct 2, 2015

Conversation

dinodsaurus
Copy link
Contributor

I just added check if the component is running in browser or node.
Before when rendering on server there was an error

/Users/dino/Documents/dino/react-modal/lib/helpers/ariaAppHider.js:4
if (document) {
    ^

ReferenceError: document is not defined

@tkirda
Copy link

tkirda commented Sep 25, 2015

Yes, that is needed! Now it fails when rendering isomorphic app. I suggest changing first line into:

var _element = typeof document !== 'undefined' ? document.body : null;

There is no point setting _element to string.

@dinodsaurus
Copy link
Contributor Author

Thanks @tkirda , I updated my pull request 👍

@Josh-a-e
Copy link

Would love to see this merged in 😄

@sebslomski
Copy link

Please merge this, it works perfectly.

@ghislaindj
Copy link

Totally agree, this PR works great !

@voronianski
Copy link

rackt guys @gaearon @mzabriskie please merge this line and bump npm version pleeeeeaseee ⌚

@mzabriskie
Copy link
Member

Has anyone tested this diff with a screen reader? The fix would work for server rendering. Just need to make sure that once we're on the client _element still gets aria-hidden="true" when modal is displayed.

@tkirda
Copy link

tkirda commented Oct 2, 2015

It does not affect the behavior in any way, it's just to prevent exception when including file in nodejs runtime. Since document is not available. On a client it will behave the same way.

@mzabriskie
Copy link
Member

Sounds good. Thanks!

mzabriskie added a commit that referenced this pull request Oct 2, 2015
added suport for isomorphic rendering
@mzabriskie mzabriskie merged commit 920d421 into reactjs:master Oct 2, 2015
@tkirda
Copy link

tkirda commented Oct 2, 2015

Sweet! Looking forward to see it on NPM.

@voronianski
Copy link

@mzabriskie 👍

@armandabric
Copy link

Any idea on when a new release will be?

@mzabriskie
Copy link
Member

I will update react to 0.14, and release today.

@sahat
Copy link

sahat commented Oct 15, 2015

Has this been published yet? I am still having this issue in version 0.5.

@vonwao
Copy link

vonwao commented Oct 20, 2015

Still doesn't work with meteor.

@vonwao vonwao mentioned this pull request Oct 20, 2015
seriallos pushed a commit to seriallos/react-modal that referenced this pull request Oct 26, 2015
@ericntd-legacy
Copy link

It looks like the fix is included in 0.6.0.
But then 0.6.0 requires React 0.14 and I'm using React 0.13.3.
What else can I do besides crying and changing the source code of your library under node_modules folder (hacking)?

@dinodsaurus
Copy link
Contributor Author

@ericntd In my package.jsonI am loading it as this:
"react-modal": "git+https://github.com/rackt/react-modal.git#920d421ed14b94b47bb32310eb691f763d67d464"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.