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

Use comment nodes for empty components #5451

Merged
merged 1 commit into from Nov 20, 2015

Conversation

Projects
None yet
8 participants
@sophiebits
Copy link
Collaborator

commented Nov 11, 2015

This makes more sense and avoids DOM nesting problems.

image

(ReactSimpleEmptyComponent isn't used here but React Native can use it as it currently does, with View.)

Use comment nodes for empty components
This makes more sense and avoids DOM nesting problems.

![image](https://cloud.githubusercontent.com/assets/6820/11098713/952348ca-885b-11e5-9757-e4a76467b0b8.png)

(ReactSimpleEmptyComponent isn't used here but React Native can use it as it currently does.)
@probablyup

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2015

@spicyj I was hoping this would come to pass. I remember it was discussed and ultimately postponed by @syranide (I believe) because of some IE whitespace issues. Have those been resolved or obviated by removing support for old IE?

@sophiebits

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 11, 2015

Only reference I can find is #4550 which shouldn't actually be an issue because we work around it in our setInnerHTML module.

@probablyup

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2015

👍 excellent

@mwiencek

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2015

With unencrypted HTTP, you'll need to set a Cache-Control: no-transform response header if you don't want certain mobile providers to strip comments. This is a problem we had with Knockout.

@sophiebits

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 11, 2015

I thought Knockout is a client-side library only?

@sophiebits sophiebits closed this Nov 11, 2015

@sophiebits sophiebits reopened this Nov 11, 2015

@mwiencek

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2015

Yup, though Knockout bindings are just attributes/comments in the HTML, so it was common to just render the initial page containing all the knockout-y bits with whatever server-side templating language you were using, then call applyBindings once the page loaded to activate them.

@sophiebits

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 11, 2015

Got it – thanks for the explanation! I don't think this will be an issue for us.

* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @providesModule ReactSimpleEmptyComponent

This comment has been minimized.

Copy link
@zpao

zpao Nov 19, 2015

Member

You don't use this anywhere… is the thinking that this would be used in RN?

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 19, 2015

Author Collaborator

Yeah. (I even mentioned that in my PR description. ;))

@@ -166,7 +166,7 @@ var Danger = {
);
invariant(markup, 'dangerouslyReplaceNodeWithMarkup(...): Missing markup.');
invariant(
oldChild.tagName.toLowerCase() !== 'html',
oldChild.nodeName !== 'HTML',

This comment has been minimized.

Copy link
@zpao

zpao Nov 19, 2015

Member

https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeName indicates that nodeName should be 'html' for XHTML docs but that doesn't appear to be true in practice (for several years, the linked article there is from 2009) so this is probably safe.

sophiebits added a commit that referenced this pull request Nov 20, 2015

Merge pull request #5451 from spicyj/empty-comments
Use comment nodes for empty components

@sophiebits sophiebits merged commit 907dee2 into facebook:master Nov 20, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zpao

This comment has been minimized.

Copy link
Member

commented Nov 20, 2015

doit

@newoga newoga referenced this pull request Feb 18, 2016

Merged

[Core] Remove click-awayable mixin #3360

4 of 4 tasks complete
@martijnrusschen

This comment has been minimized.

Copy link

commented Feb 22, 2016

Interesting! Would really like to use this, but it looks like this code hasn't been released yet. Any idea why this will be released?

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2016

@martijnrusschen This will be released in v15, coming soon.

@martijnrusschen

This comment has been minimized.

Copy link

commented Feb 22, 2016

👍

@alxndr

This comment has been minimized.

Copy link

commented Oct 11, 2016

Should this mean that React can render <noscript> elements normally, without the need for workarounds like what powers the react-noscript package?

@sophiebits

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 11, 2016

@alxndr That's unrelated, sorry.

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.