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

[#10166] Highlight 'text/ng-template' (Angular) in script tags as HTML #10666

Merged
merged 2 commits into from Mar 7, 2015

Conversation

Projects
None yet
4 participants
@munxtwo
Contributor

munxtwo commented Mar 1, 2015

No description provided.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Mar 1, 2015

Member

@munxtwo

  • You'll need to sign the Brackets CLA before this can be merged
  • Should it really apply to any mimetype containing the string ng-, or just ng-template specifically?
Member

peterflynn commented Mar 1, 2015

@munxtwo

  • You'll need to sign the Brackets CLA before this can be merged
  • Should it really apply to any mimetype containing the string ng-, or just ng-template specifically?
@munxtwo

This comment has been minimized.

Show comment
Hide comment
@munxtwo

munxtwo Mar 1, 2015

Contributor

@peterflynn

I have just signed the brackets cla.

Regarding your question, I was trying to follow the convention for the other mime types in the regex which did not include the "-template". However, on further thought, since "ng" is somewhat generic, it would probably be better to specify "ng-template" instead. I will change it soon. Thanks.

Contributor

munxtwo commented Mar 1, 2015

@peterflynn

I have just signed the brackets cla.

Regarding your question, I was trying to follow the convention for the other mime types in the regex which did not include the "-template". However, on further thought, since "ng" is somewhat generic, it would probably be better to specify "ng-template" instead. I will change it soon. Thanks.

@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip Mar 5, 2015

Contributor

@munxtwo Please make the required changes suggested by @peterflynn so that we can merge this to master.

Contributor

nethip commented Mar 5, 2015

@munxtwo Please make the required changes suggested by @peterflynn so that we can merge this to master.

@munxtwo

This comment has been minimized.

Show comment
Hide comment
@munxtwo

munxtwo Mar 5, 2015

Contributor

@nethip I already made the changes 4 days ago. are they not there?

Contributor

munxtwo commented Mar 5, 2015

@nethip I already made the changes 4 days ago. are they not there?

@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip Mar 5, 2015

Contributor

@munxtwo okay I was thinking the change was from ng-template to ng. So the correct thing is the other way round. Could you share any unit testing you have done around this change? Thanks

Contributor

nethip commented Mar 5, 2015

@munxtwo okay I was thinking the change was from ng-template to ng. So the correct thing is the other way round. Could you share any unit testing you have done around this change? Thanks

@munxtwo

This comment has been minimized.

Show comment
Hide comment
@munxtwo

munxtwo Mar 5, 2015

Contributor

@nethip i did not write any unit tests for this. I was not able to find any that were testing specifically around this. If you could point me to an example that would be great. Thanks.

Contributor

munxtwo commented Mar 5, 2015

@nethip i did not write any unit tests for this. I was not able to find any that were testing specifically around this. If you could point me to an example that would be great. Thanks.

@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip Mar 5, 2015

Contributor

@munxtwo If you can just share what unit testing you have done that should be fine too. Having a written unit test is surely good to have, as other people can just run tests whenever some changes go in this area.

By the way our test suite is present at the following location

https://github.com/adobe/brackets/tree/master/test/spec/

You can look at the JS files at this location to get an understanding of how to write tests in Brackets.

Contributor

nethip commented Mar 5, 2015

@munxtwo If you can just share what unit testing you have done that should be fine too. Having a written unit test is surely good to have, as other people can just run tests whenever some changes go in this area.

By the way our test suite is present at the following location

https://github.com/adobe/brackets/tree/master/test/spec/

You can look at the JS files at this location to get an understanding of how to write tests in Brackets.

@munxtwo

This comment has been minimized.

Show comment
Hide comment
@munxtwo

munxtwo Mar 6, 2015

Contributor

@nethip oh were you referring to the manual test cases that I have run?

I basically used the code snippet that was provided in the issue description. Here are the test cases I did:

  1. type="text/ng-template"
    Verified that code in between script tags are highlighted as html.
  2. type="text/Xng-template"
    Verified that code in between script tags are not highlighted as html.
  3. type="ng-templateX"
    Verified that code in between script tags are not highlighted as html.
Contributor

munxtwo commented Mar 6, 2015

@nethip oh were you referring to the manual test cases that I have run?

I basically used the code snippet that was provided in the issue description. Here are the test cases I did:

  1. type="text/ng-template"
    Verified that code in between script tags are highlighted as html.
  2. type="text/Xng-template"
    Verified that code in between script tags are not highlighted as html.
  3. type="ng-templateX"
    Verified that code in between script tags are not highlighted as html.
@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Mar 7, 2015

Member

@munxtwo Thanks, this looks good. As you mentioned, we don't have any existing unit tests for this nested mode selection (it mostly leans on CodeMirror code anyway), so I don't think we need any for this PR.

Member

peterflynn commented Mar 7, 2015

@munxtwo Thanks, this looks good. As you mentioned, we don't have any existing unit tests for this nested mode selection (it mostly leans on CodeMirror code anyway), so I don't think we need any for this PR.

peterflynn added a commit that referenced this pull request Mar 7, 2015

Merge pull request #10666 from munxtwo/issue-10166
[#10166] Highlight 'text/ng-template' (Angular) in script tags as plain HTML

@peterflynn peterflynn merged commit cad2675 into adobe:master Mar 7, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Show outdated Hide outdated src/language/LanguageManager.js
@@ -1067,7 +1067,7 @@ define(function (require, exports, module) {
// far were strings, so we spare us the trouble of allowing more complex mode values.
CodeMirror.defineMIME("text/x-brackets-html", {
"name": "htmlmixed",
"scriptTypes": [{"matches": /\/x-handlebars|\/x-mustache|^text\/html$/i,
"scriptTypes": [{"matches": /\/x-handlebars|\/x-mustache|\/ng-|^text\/html$/i,

This comment has been minimized.

@maximelebreton

maximelebreton Aug 30, 2017

and x-template for vue.js?

@maximelebreton

maximelebreton Aug 30, 2017

and x-template for vue.js?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment