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

Fix HTML hinting issues caused by new token types -- "error" and "tag error". #5489

Merged
merged 2 commits into from
Oct 14, 2013

Conversation

RaymondLim
Copy link
Contributor

"error" was introduced in codemirror/codemirror5@32abe85 and then caused #5343. "tag error" was introduced in codemirror/codemirror5@a6c2e01 and will break tag hinting when we update CodeMirror for sprint-33. Tag hints won't show up when you type a <. So this needs to land in master before landing the next CodeMirror update.

@redmunds
Copy link
Contributor

Done with review. I verified that this fixes the bug.

But I'm concerned that the token parsing code keeps getting more and more complicated. I also see many places in CSSUtils.js that is checking for [context].token.type === "tag". Could that code also be affected?

Can you think of any methods that could be added to the TokenUtils API that could simplify your code, and also make it easier for other Devs to use the Tokenizer?

@RaymondLim
Copy link
Contributor Author

@redmunds I agree that the token parsing code becomes more complicated. This is unavoidable as we rely on CodeMirror and we just have to make all the corresponding changes. Thanks for pointing out token type "tag" used in CSSUtils.js. I reviewed all of them used in that file and also tested with < character in CSS selector location where we're getting token type "tag". I didn't get any error token type as we have in HTML.

It's possible that we can refactor some of the current logic into either local helper functions or TokenUtils API if that makes sense, but it should be done when we have time to do some cleanup.

@redmunds
Copy link
Contributor

Merging.

redmunds added a commit that referenced this pull request Oct 14, 2013
Fix HTML hinting issues caused by new token types -- "error" and "tag error".
@redmunds redmunds merged commit dfe45f4 into master Oct 14, 2013
@redmunds redmunds deleted the rlim/hint-issue-5343 branch October 14, 2013 21:55
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.

2 participants