-
Notifications
You must be signed in to change notification settings - Fork 913
PHP: Added CSS embedding to heredoc/nowdoc #9074
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
base: master
Are you sure you want to change the base?
Conversation
ide/editor.indent/src/org/netbeans/modules/editor/indent/TaskHandler.java
Outdated
Show resolved
Hide resolved
...hp.editor/src/org/netbeans/modules/php/editor/embedding/CssEmbeddingHighlightsContainer.java
Show resolved
Hide resolved
...hp.editor/src/org/netbeans/modules/php/editor/embedding/CssEmbeddingHighlightsContainer.java
Show resolved
Hide resolved
php/php.editor/src/org/netbeans/modules/php/editor/embedding/CssEmbeddingProvider.java
Outdated
Show resolved
Hide resolved
php/php.editor/src/org/netbeans/modules/php/editor/lexer/GSFPHPLexer.java
Outdated
Show resolved
Hide resolved
tmysik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid that I am not able to fully verify the editor-related implementations but overall, the change looks good to me, thank you.
d8a066a to
568b554
Compare
|
@tmysik I've made the corrections. |
|
@troizet thanks! |
568b554 to
df565ff
Compare
|
Updated the pull request. Fixed the slowdown when opening PHP syntax highlighting settings. |
|
@troizet in general this looks very nice. I have a concern about scalability of the approach. I tripped over the modification to the lexer structure and the introduction of a special tokenid for CSS. Given that the referenced ticket already asks for HTML, SQL and JSON I see this growing in the future. Please have a look at this illustration for the idea: The use of the property token allows us to transfer the info from the lexer to the analyser without changing the token ids. New mapping would just report a different mimetype. This is currently considered an implementation dependency so the property was marked appropriately and the constant defined outside the exported classes. What do you think? |
Great idea! I'll try to make some changes as soon as possible. Thanks! |
Added
CSSembedding toheredoc/nowdoc.Partially implements feature request #5010.
Before:

After:

Based on discussion #7933, I was able to piece together the puzzle and add
CSSembedding toheredocandnowdoc.The
CssEmbeddingProviderand test code is borrowed from theJavascript2 VUEmodule.The background highlighting code is borrowed from the
HTML Editormodule.I also encountered a formatter execution order issue, which was described by @matthiasblaesing in commit be093c3. I solved this issue by adding a hack to TaskHandler.java:
https://github.com/troizet/netbeans/blob/f9f1f9fe22e4a46f4910a476449725c4a72fbb7f/ide/editor.indent/src/org/netbeans/modules/editor/indent/TaskHandler.java#L170-L186
^Add meaningful description above
Click to collapse/expand PR instructions
By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -
Please make sure (eg.
git log) that all commits have a valid name and email address for you in the Author field.If you're a first time contributor, see the Contributing guidelines for more information.
If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.
PR approval and merge checklist:
If this PR targets the delivery branch: don't merge. (full wiki article)