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

Fixed emmet validation when open angle bracket is followed by space #55762

Merged

Conversation

mathdeziel
Copy link
Contributor

@mathdeziel mathdeziel commented Aug 3, 2018

Fix for issue #55411.

Much like the check for PHP's <?, it checks for an open angle bracket followed by a space, which is not valid as an HTML Tag, based on the W3C recommendation.

This will fix issues where, for example, emmet won't trigger in JSX when the if statement contains a <, eg: if (age < 10) ....

This will not work if the there is no space after the angle bracket, as in if (age <10), as numbers are valid within tag names. But this problem should be taken care of by linting.

@mathdeziel
Copy link
Contributor Author

I am not used to your standards yet and the coding guidelines didn't mention anyting in regard to that, but I decided to add another condition instead of adding an OR to the existing PHP check just to have more explicit checks.

Of course this could easily be refactored as if ((char === question || char === space) && textToBackTrack[i] === startAngle) if it is an issue.

@JacksonKearl
Copy link
Contributor

Thanks! If you could just add a test here it should be good to go.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

The same applies if there is a \t instead of a space. I'd suggest to use /\s/.test(char) instead.

@mathdeziel
Copy link
Contributor Author

Great suggestion @ramya-rao-a! I just pushed this modification.

Now I'm trying to figure out how those automated tests work and I'll push one @JacksonKearl.

@JacksonKearl
Copy link
Contributor

@mathdeziel You can run the tests with the "VS Code Emmet Tests" debug configuration, and you can use standard mocha constructs like suite.only to restrict the total number of tests run/noise generated.

@msftclas
Copy link

msftclas commented Aug 3, 2018

CLA assistant check
All CLA requirements met.

@mathdeziel
Copy link
Contributor Author

Thanks for the tip @JacksonKearl!

I just pushed a test, I hope it's properly done. It was my first time using Mocha. Truth be told, it was my first time for a lot of things on this journey...

Thanks for your help/support @JacksonKearl, @ramya-rao-a 😄

@ramya-rao-a ramya-rao-a added this to the August 2018 milestone Aug 3, 2018
Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

Thanks @mathdeziel. We are currently at the end of the testing phase for the current iteration. So, I'll merge this sometime next week for the next iteration.

Note to anyone else: Please dont merge this PR until the release branch for 1.26 has been cut

@ghost
Copy link

ghost commented Aug 3, 2018 via email

@ramya-rao-a ramya-rao-a merged commit f8420b4 into microsoft:master Aug 5, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants