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

Remove deprecation warning in react 15.6.0 about React.DOM.noscript #410

Closed
wants to merge 1 commit into from
Closed

Conversation

ingro
Copy link
Contributor

@ingro ingro commented Jun 14, 2017

As the title, this change remove the soon-will-be-deprecated use of React.DOM factories with the new drop-in package react-dom-factories instead.

I also added cross-env package to be able to run tests on windows.

Upgrade Path (for changed or removed APIs):
Nothing needed.

Acceptance Checklist:

  • All commits have been squashed to one.
  • The commit message follows the guidelines in CONTRIBUTING.md.
  • Documentation (README.md) and examples have been updated as needed.
  • If this is a code change, a spec testing the functionality has been added.
  • If the commit message has [changed] or [removed], there is an upgrade path above.

@diasbruno
Copy link
Collaborator

diasbruno commented Jun 14, 2017

Hi @ingro, can you have a look at PR #395? Hope we ca use it to simplify a little bit (the portal mechanism).

@ingro
Copy link
Contributor Author

ingro commented Jun 14, 2017

Hello @diasbruno , I wasn't aware of that PR, your solution is surely better on the long-term, I wanted just to get rid of a deprecation notice! :P

@diasbruno
Copy link
Collaborator

diasbruno commented Jun 14, 2017

Yeah, that's ok. :)
Do you mind help me with some tests? I think it just need to be tests on react@0.14.

Copy link
Collaborator

@diasbruno diasbruno left a comment

Choose a reason for hiding this comment

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

I think you just need to remove the cross-env and we are good to make a release with this. Thank you.

@@ -14,7 +14,7 @@
},
"scripts": {
"start": "./node_modules/.bin/webpack-dev-server --inline --host 127.0.0.1 --content-base examples/",
"test": "NODE_ENV=test karma start"
"test": "cross-env NODE_ENV=test karma start"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is out of scope.

@andrewjmead
Copy link

andrewjmead commented Jun 15, 2017

While the warning about React.DOM.noscript is gone, there is still a warning related to React.DOM.div. Both are related to the new react-dom-factories depreciation. It's coming from this.

You can fix this by modifying lib/components/ModalPortal.js:

// Require lib
var DOMFactories = require('react-dom-factories');

// Then replace this
var div = React.DOM.div;
// With this
var div = DOMFactories.div;

New warning

Warning: Accessing factories like React.DOM.noscript has been deprecated and will be removed in v16.0+. Use the react-dom-factories package instead.  Version 1.0 provides a drop-in replacement.

Previous Warning

Warning: Accessing factories like React.DOM.div has been deprecated and will be removed in v16.0+. Use the react-dom-factories package instead.  Version 1.0 provides a drop-in replacement. For more info, see https://fb.me/react-dom-factories

@diasbruno
Copy link
Collaborator

diasbruno commented Jun 15, 2017

Released v1.9.6.
@ingro I've added @andrewjmead changes to your PR, removed cross-env and fixed both test and example.
Your patch is 91e1a67.
Thank you.

@diasbruno diasbruno closed this Jun 15, 2017
@ingro
Copy link
Contributor Author

ingro commented Jun 15, 2017

Thank you! Sorry but I couldn't reply earlier!

@diasbruno
Copy link
Collaborator

No problem, @ingro. We are close to get v2.0.0 merged, so I'm trying to get the last PRs merged, because I always have to rebase #412. :)
Please, open a PR for the cross-env.

@ingro
Copy link
Contributor Author

ingro commented Jun 15, 2017

Done! :D

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.

None yet

3 participants