Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Conversation

@munxtwo
Copy link
Contributor

@munxtwo munxtwo commented Mar 1, 2015

No description provided.

@peterflynn
Copy link
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?

@munxtwo
Copy link
Contributor Author

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
Copy link
Contributor

nethip commented Mar 5, 2015

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

@munxtwo
Copy link
Contributor Author

munxtwo commented Mar 5, 2015

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

@nethip
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
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.

peterflynn added a commit that referenced this pull request Mar 7, 2015
[#10166] Highlight 'text/ng-template' (Angular) in script tags as plain HTML
@peterflynn peterflynn merged commit cad2675 into adobe:master Mar 7, 2015
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,

Choose a reason for hiding this comment

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

and x-template for vue.js?

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