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

JSX: Add partial support for plain text between tags #1304

Closed
wants to merge 1 commit into from

Conversation

Golmote
Copy link
Contributor

@Golmote Golmote commented Feb 21, 2018

This is an attempt at a better support for plain text between tags in JSX (and incidently TSX). See #1294.

Plain text will be matched between an opening tag (<xxx>) and the next tag encountered (<xxx>, </xxx> or <xxx/>).

This behaviour gives decent results IMO, but is not perfect.
It will fail on those examples:

var a = <div>
    this is plain text
    <div>this too</div>
    this is not considered as plain text anymore
</div>;

kpbv8rr

var b = <div>
    this is plain text
    <img src="foo.png" />
    this is not considered as plain text anymore
</div>;

slbib2z

The thing that worries me is that with this improvement, those look even more like bugs than unhandled cases.

What do you think? @m-allanson, @LeaVerou, @mAAdhaTTah, @vkbansal

@mAAdhaTTah
Copy link
Member

The thing that worries me is that with this improvement, those look even more like bugs than unhandled cases.

Just to clarify what you mean, you're worried that because the first batch of plaintext ("this is plaintext" in ur screenshot) doesn't highlight "this", the second batch that does looks more like a bug?

@Golmote
Copy link
Contributor Author

Golmote commented Feb 22, 2018

Yes, it's exactly what I mean. It's clearer, expressed like this, thanks ^^

@LeaVerou
Copy link
Member

Yeah, that’s definitely a bug…
What if we have the tag pattern be matched before keywords?

@Golmote
Copy link
Contributor Author

Golmote commented Feb 23, 2018

@LeaVerou well no it's not a bug, that's precisely my point here. It is an case that is explicitly not handled by the behaviour I described in the first post.
But it does look like a bug.

Changing the order of the patterns won't help here AFAICT. But maybe there is a better algorithm...

@LeaVerou
Copy link
Member

Oh I see, you fixed the highlighting for some of the plain text, but not all cases!
Sorry, I hadn’t seen the linked issue before.

@LeaVerou
Copy link
Member

LeaVerou commented Feb 24, 2018

I’m pretty late in the game so I might be suggesting silly ideas that you've already considered, but instead of merging the two languages like this (where you have no control over how it works), have you considered using the JS def (cloned obvs), adding a token to it for the tags (before keywords) and then matching HTML inside it?
Isn't that how we usually do embedded languages?

@Golmote
Copy link
Contributor Author

Golmote commented Feb 24, 2018

I'm afraid we would face the same issue (although I agree the way the component is done currently differs from our other embedded languages).

Currently, the tag pattern is already before the keyword pattern (since Markup is extended with JS and not the opposite). But the tag pattern only matches the part between an opening and a closing brackets (< >).
Here, we need to match the contents of the tags, and this can include an infinitely deep nesting of children tags.

Quick list of ideas I already tried:

  • We could match between an opening tag and a corresponding closing tag, but this would still fail on nested <div>s for example. And it would require using rest to recursively highlight the inner tags that would have been included in the plain-text match.
  • We could match between any tags (opening or closing), but this would be too greedy and would fail on a = <div>foo</div>; b = <div>bar</div>; think ; b = is plain text.

I honestly think we can't get a perfect result here. So the following question would be: is it worth adding partial support as shown in the screenshots in the PR, or it is better if we assume Prism can't match plain text in JSX?

@Golmote
Copy link
Contributor Author

Golmote commented Mar 15, 2018

Closing this PR in favor of #1357.

@Golmote Golmote closed this Mar 15, 2018
@Golmote Golmote deleted the jsx-plain-text branch March 20, 2018 19:58
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