Skip to content
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

Fix hovers and conflicts in Vue <style lang="sass"> blocks #930

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

thecrypticace
Copy link
Contributor

We weren't parsing these blocks as Sass (or any semicolon-less language) and we were instead assuming CSS syntax. Because of this we wouldn't pick up conflicts when we should and sometimes would pick up conflicts when we shouldn't.

This PR fixes this by handling language boundaries on hover and threading language information from <style lang="…"> blocks.

Fixes #895

@RobinMalfait
Copy link
Member

Do you think it's a lot of work to setup CI for tests? I can't figure out if these tests are expected to fail or not:

image

@RobinMalfait
Copy link
Member

Ah figured it out, I had to manually go into each tests/fixtures/v{1,4} and run npm install. I think this is something we should improve in a future PR to make sure that all those sub folders are part of the workspace if possible.

): DocumentClassList[] {
const text = getTextWithoutComments(doc, 'css', range)
let regex = isSemicolonlessCssLanguage(doc.languageId, state.editor?.userLanguages)
let regex = isSemicolonlessCssLanguage(lang ?? doc.languageId, state.editor?.userLanguages)
Copy link
Member

Choose a reason for hiding this comment

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

Just a question, in what scenario would lang and doc.languageId be different?

is that when you have <style lang="sass"> in an html or vue file where lang will be sass and doc.languageId would be html?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vue file with a <style> block or a <script> block. Also HTML file with a <script> block. doc always refers to the whole file/document rather than just a portion of the file.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha yep that's what I meant and thought. Thanks for verifying!

@thecrypticace
Copy link
Contributor Author

Ah figured it out, I had to manually go into each tests/fixtures/v{1,4} and run npm install. I think this is something we should improve in a future PR to make sure that all those sub folders are part of the workspace if possible.

We can't make them a part of the normal project workspace because of dependency hoisting, unfortunately. If there's a way to disable hoisting but still use workspaces I'd use them for all of these.

@thecrypticace
Copy link
Contributor Author

I need to fix the test:prepare script though, good call. I'll do that in a separate PR.

@thecrypticace thecrypticace merged commit f7534ae into master Mar 22, 2024
@thecrypticace thecrypticace deleted the fix/vue-style-sass branch March 22, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSS conflicts in @apply don't handle .sass syntax
2 participants