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

Coalesce adjacent strings into one text node #742

Closed
wants to merge 2 commits into from

Conversation

sophiebits
Copy link
Collaborator

Depends on #741.

Should make #480 a little less painless. This brings us a lot closer to pretending we're in a world where text components don't exist at all; I think this is a lot nicer overall.

With this, <div>{['A', ' ', 'B']}</div> turns into <div>A B</div> instead of <div><span>A</span><span> </span><span>B</span></div>.

The only "weird" thing that I can see here is that <div>x{flag && <a />}</div> will toggle between rendering to <div>x</div> and <div><span>x</span><a /></div>.

This simplifies ReactMultiChild a little bit and will make it more practical to coalesce adjacent text strings.
@syranide
Copy link
Contributor

👍 x10

Should make facebook#480 a little less painless. This brings us a lot closer to pretending we're in a world where text components don't exist at all; I think this is a lot nicer overall.

With this, `<div>{['A', ' ', 'B']}</div>` turns into `<div>A B</div>` instead of `<div><span>A</span><span> </span><span>B</span></div>`.

The only "weird" thing that I can see here is that `<div>x{flag && <a />}</div>` will toggle between rendering to `<div>x</div>` and `<div><span>x</span><a /></div>`.
@sebmarkbage
Copy link
Collaborator

If you use ReactChildren it won't coalesce later since it turns them into ReactTextComponent first. It would be nice to keep them as strings until they're mounted I guess.

I'm not sure if it's worth while thrashing on these edge cases until we can get a consistent experience where we're never wrapping with spans and can clean up all the ReactTextComponent weirdness.

@sebmarkbage
Copy link
Collaborator

I'll close this out but this is a great idea. If we can do it in multichild, we could get rid of spans completely. I think we can do it. That change would be easier to reason about since it just means that we never wrap things in weird ways.

@sophiebits
Copy link
Collaborator Author

Agree, will look at doing that when I have time.

@thSoft
Copy link

thSoft commented Feb 1, 2014

This would be great also because the superfluous spans interfere badly with onMouseOver events: they "steal" these from their parent elements, which is very counterintuitive for the user. :(

@sophiebits
Copy link
Collaborator Author

@thSoft Good catch. I still want to get rid of the spans completely and think that we can do it.

@sebmarkbage
Copy link
Collaborator

The solution is to use onMouseEnter. This is why we had onMouseOver disabled before because it gives confusing effects in certain circumstances.

@thSoft
Copy link

thSoft commented Feb 2, 2014

But hovering the topmost element can be only done with onMouseOver/Out
AFAIK. My workaround is to explicitly create spans with style
pointerEvents: "none" for these text nodes.
2014.02.02. 6:31 ezt írta ("Sebastian Markbåge" notifications@github.com):

The solution is to use onMouseEnter. This is why we had onMouseOver
disabled before because it gives confusing effects in certain circumstances.

Reply to this email directly or view it on GitHubhttps://github.com//pull/742#issuecomment-33892832
.

@natew
Copy link

natew commented Nov 12, 2015

@spicyj @sebmarkbage any plans on reviving this? Just curious.

@facebook-github-bot
Copy link

@spicyj updated the pull request.

@sophiebits
Copy link
Collaborator Author

(What @facebook-github-bot? No I didn't.)

@natew I'd like to fix this sometime but it hasn't been a priority.

@jimfb
Copy link
Contributor

jimfb commented Nov 12, 2015

@natew @spicyj @facebook-github-bot Pure speculation: if there is some activity (any activity) on an old PR that predates @facebook-github-bot (perhaps a PR that doesn't have a review label), then github bot doesn't know about it and so it claims the PR was updated.

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.

8 participants