You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository was archived by the owner on Sep 6, 2021. It is now read-only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Update: Removed "[discussion only]" tag since we think we've addressed the issues and could merge this.
In the latest CM (as of codemirror/codemirror5@d13582f), the CSS mode has been refactored substantially. The good news is that LESS is now supported by CSS mode, meaning that we can much more easily make basic code hinting work for LESS files (similar to what we've already done for SCSS files). The less good news is that the internals of the mode have changed significantly. In particular:
There's no longer a stack member on the state object, but instead a context member which seems to fulfill the same role (using a linked list instead of a stack).
Some of the token types appear to have changed (specifically, propertyValue now seems to be prop).
This PR contains a quick attempt to make the existing CSS code hinting work properly with the changes to CSS mode. It also hooks up LESS files to the CSS mode (instead of the old LESS mode) and attempts to turn on code hinting in those files.
With these changes, it looks like almost all the existing CSS code hinting unit tests pass, except for one in CSS Context Info. However, I haven't done much manual testing beyond verifying that basic property and value hints seem to work in CSS and LESS files. Also, this was based on only a very cursory understanding of the existing CSSUtils code and of the changes to the CM CSS mode, so it's quite likely that we need to do more work and testing to make sure everything is working properly.
@RaymondLim - would you mind taking a look at the code changes and seeing what you think? I'm wondering if we need to add a story for this (to ensure we get adequate testing, etc.), which might mean pushing it to the next sprint.
Additionally, there were a number of other smaller changes since our last CM merge that are probably worth testing around. From looking at the commit log, these are the areas we'd want to do some sanity testing around:
Inline editor line hiding (esp at beginning/end of doc) - make fix first
LESS - using CSS mode now! - include that and test
Just realized that codemirror/codemirror5@fad3f49, where Marijn updated his own CSS code hinting add-on to work with the new mode, might give us clues as to the right way to fix our code hinting. In particular, it looks like he uses token.state.state instead of token.state.context.type (seems like the two might be equivalent). You probably still need to use context.prev.type to get the previous state though.
Also, I just realized that I don't really know what the difference is between token.type and token.state.state (or token.state.context.type). If the two are always equivalent, maybe we should just be checking token.type?
Good to know. I didn't run all the unit tests myself so it's not too surprising there are other failures. I have a task on the Sprint 36 CM Merge card to look into these, but I think we're waiting until Jonathan makes a call on whether to try to pull this work in. Until then, maybe we should just put this on hold.
I think I've fixed all unit test failures. The remaining failures -- 1 in Bower-install and 1 or 2 in JS code hints -- have nothing to do with CM update and can be reproduced in master as well. I filed an issue for the HTML Code hints unit test failure and Marijn already fixed it in codemirror/codemirror5@08d5cab, but I don't think we can include the fix in this pull request. So I just disable the unit test for now.
Makes sense. However, we should file a bug on the case related to the disabled unit test. It looks like it causes an obvious regression in the code coloring (as in the bug you filed), but I wasn't able to actually reproduce any problems with code hints in that case. It's a bit of a bummer to ship with the code-coloring regression, but we can release note it.
I did notice that the fix for the issue is only in the v4 branch of CodeMirror for some reason - I left him a comment asking about that.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
None yet
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ghost
assigned
Update: Removed "[discussion only]" tag since we think we've addressed the issues and could merge this.
In the latest CM (as of codemirror/codemirror5@d13582f), the CSS mode has been refactored substantially. The good news is that LESS is now supported by CSS mode, meaning that we can much more easily make basic code hinting work for LESS files (similar to what we've already done for SCSS files). The less good news is that the internals of the mode have changed significantly. In particular:
stackmember on thestateobject, but instead acontextmember which seems to fulfill the same role (using a linked list instead of a stack).propertyValuenow seems to beprop).This PR contains a quick attempt to make the existing CSS code hinting work properly with the changes to CSS mode. It also hooks up LESS files to the CSS mode (instead of the old LESS mode) and attempts to turn on code hinting in those files.
With these changes, it looks like almost all the existing CSS code hinting unit tests pass, except for one in CSS Context Info. However, I haven't done much manual testing beyond verifying that basic property and value hints seem to work in CSS and LESS files. Also, this was based on only a very cursory understanding of the existing CSSUtils code and of the changes to the CM CSS mode, so it's quite likely that we need to do more work and testing to make sure everything is working properly.
@RaymondLim - would you mind taking a look at the code changes and seeing what you think? I'm wondering if we need to add a story for this (to ensure we get adequate testing, etc.), which might mean pushing it to the next sprint.
Additionally, there were a number of other smaller changes since our last CM merge that are probably worth testing around. From looking at the commit log, these are the areas we'd want to do some sanity testing around:
Finally, this PR also contains the small update required by #6204.