Render Component that returns null #195

sdoomz · 2016-11-17T08:00:27Z

Since React 15 has been released no more <noscript> tag needed

gajus · 2016-11-17T09:43:10Z

This would be a breaking change. Cannot merge this without (a) introducing a peerDependency setting to package.json and releasing a new major.

In the mean time, this does not break anything in React v0.15, therefore it is safe to leave this PR hanging until general adoptoption of the lastest React version.

Thank you

sdoomz · 2016-11-17T09:59:04Z

Currently :last-child pseudo-selector doesn't work as expected after applying CSSModules.

Last component wrapped with CSSModules:

<div>
  <button></button>
  <noscript/>
</div>

Last component is a pure react function

<div>
   <button></button>
   <!-- react-empty: 1 -->
</div>

Thank you for your fast reaction

gajus · 2016-11-17T10:02:37Z

This behaviour could be made conditional. I think. Need to think more about the implications.

ipanasenko · 2016-11-17T11:24:23Z

What do you think if we check React version and return <noscript> or null correspondingly?

gajus · 2016-11-17T12:07:12Z

What do you think if we check React version and return <noscript> or null correspondingly?

Thats what I am thinking too. I suppose it is reasonable that developer's styling can break in this instance when updating the React version. Albeit, it is confusing since the break is being introduced by react-css-modules.

However, in this case, I think that the pros outweigh the cons.

sdoomz · 2016-11-21T02:11:59Z

@gajus what do you think about such kind of fix?

ipanasenko

You should also add tests to check different React versions

ipanasenko · 2016-11-21T11:19:54Z

src/renderNothing.js

+import React from 'react';
+
+export default function () {
+  const major = React.version.split('.')[0];


You really need to calculate it once. And you can check if it's < 15 only once too.

I'am not fully confident that cache is 100% safe for this case. Someone might want to use different react versions with cssModules, isn't he?

you are checking version of React that you import yourself, it's not going to change on the fly

Yes, you are right

sdoomz · 2016-11-28T09:55:19Z

@gajus any updates for this PR?

gajus · 2016-11-28T09:59:33Z

tests/renderNothing.js

@@ -0,0 +1,22 @@
+/* eslint-disable max-nested-callbacks, react/prefer-stateless-function, class-methods-use-this */


Whats the reason for needing react/prefer-stateless-function, class-methods-use-this ?

gajus · 2016-11-28T10:01:25Z

src/renderNothing.js

+const major = React.version.split('.')[0];
+const nothing = parseInt(major, 10) < 15 ? React.createElement('noscript') : null;
+
+export default nothing;


Please reformat this to:

import React from 'react'; export default (reactVersion) => { const major = reactVersion.split('.')[0]; const nothing = parseInt(major, 10) < 15 ? React.createElement('noscript') : null; };

gajus · 2016-11-28T10:01:45Z

package.json

@@ -39,6 +39,7 @@
    "husky": "^0.11.9",
    "jsdom": "^9.8.3",
    "mocha": "^3.1.2",
+    "proxyquire": "^1.7.10",


Remove proxyquire dependency.

gajus · 2016-11-28T10:02:05Z

src/wrapStatelessFunction.js

@@ -39,7 +39,7 @@ export default (Component: Function, defaultStyles: Object, options: Object): Fu
      return linkClass(renderResult, styles, options);
    }

-    return React.createElement('noscript');
+    return nothing;


Replace with renderNothing(React.version).

Fixed and squashed

gajus · 2016-11-28T10:02:26Z

tests/renderNothing.js

+} from 'chai';
+import proxyquire from 'proxyquire';
+
+describe('renderNothing', () => {


Test renderNothing as a function.

sdoomz force-pushed the master branch from 1bc6b81 to 83e6c06 Compare November 17, 2016 09:34

sdoomz changed the title ~~Render components that returns null~~ Render Component that returns null Nov 17, 2016

gajus added the enhancement label Nov 17, 2016

sdoomz force-pushed the master branch from d5a02f4 to fcd3cb9 Compare November 19, 2016 09:41

ipanasenko suggested changes Nov 21, 2016

View reviewed changes

sdoomz force-pushed the master branch 2 times, most recently from 1aa8dd6 to eb40c80 Compare November 24, 2016 11:02

gajus requested changes Nov 28, 2016

View reviewed changes

Render components that return null for various React versions

37f10e2

sdoomz force-pushed the master branch from eb40c80 to 37f10e2 Compare November 28, 2016 10:45

gajus merged commit a9c8de2 into gajus:master Nov 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render Component that returns null #195

Render Component that returns null #195

sdoomz commented Nov 17, 2016 •

edited

Loading

gajus commented Nov 17, 2016

sdoomz commented Nov 17, 2016

gajus commented Nov 17, 2016

ipanasenko commented Nov 17, 2016 •

edited

Loading

gajus commented Nov 17, 2016

sdoomz commented Nov 21, 2016

ipanasenko left a comment

ipanasenko Nov 21, 2016

sdoomz Nov 22, 2016

ipanasenko Nov 22, 2016

sdoomz Nov 22, 2016

sdoomz commented Nov 28, 2016

gajus Nov 28, 2016

gajus Nov 28, 2016

gajus Nov 28, 2016

gajus Nov 28, 2016

sdoomz Nov 28, 2016

gajus Nov 28, 2016

		@@ -0,0 +1,22 @@
		/* eslint-disable max-nested-callbacks, react/prefer-stateless-function, class-methods-use-this */

Render Component that returns null #195

Render Component that returns null #195

Conversation

sdoomz commented Nov 17, 2016 • edited Loading

gajus commented Nov 17, 2016

sdoomz commented Nov 17, 2016

gajus commented Nov 17, 2016

ipanasenko commented Nov 17, 2016 • edited Loading

gajus commented Nov 17, 2016

sdoomz commented Nov 21, 2016

ipanasenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdoomz commented Nov 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdoomz commented Nov 17, 2016 •

edited

Loading

ipanasenko commented Nov 17, 2016 •

edited

Loading