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

Bug in highlighting #2

Open
jodaka opened this issue May 2, 2024 · 14 comments
Open

Bug in highlighting #2

jodaka opened this issue May 2, 2024 · 14 comments
Labels
wontfix This will not be worked on

Comments

@jodaka
Copy link

jodaka commented May 2, 2024

Below is an piece of SCSS and screenshot of how it looks in Zed

@import 'styles/form';

*, *:after, *:before {
  box-sizing: border-box;
}

a, p {
  word-break: break-word;
}

html {
  -webkit-font-smoothing: antialiased;
  scroll-behavior: auto;
  font-size: 4px;
  height: 100%;
}

body {
  @mixin body_long_14;
  margin: 0;
  background-color: #ffffff;
  color: var(--c-dark);
  height: 100%;
}

button {
  color: var(--c-text-default);
}

#app {
  display: flex;
  flex-direction: column;
  height: 100%;
}

selector colors are inconsistent and rules inside of body selectors seems to be completely off
image

@bajrangCoder
Copy link
Owner

Really that's not good highlighting.

Thanks for giving the piece of code , I will try to fix it as soon as possible!

@bajrangCoder bajrangCoder added the bug Something isn't working label May 2, 2024
@bajrangCoder
Copy link
Owner

I have improved the higlighting but there are some bugs in tree-sitter(I'm going to report it)

Screenshot_20240502_213610
Screenshot_20240502_213638

@xpe
Copy link

xpe commented May 5, 2024

I'm facing the same issue.

but there are some bugs in tree-sitter(I'm going to report it)

@bajrangCoder Thanks! Have you identified more about the bug? Could you link us if you share more info about it at https://github.com/tree-sitter-grammars/tree-sitter-scss/issues ?

@bajrangCoder
Copy link
Owner

@xpe sure
According to my tests , the tree-sitter doesn't support some of the scss features(as mentioned)

@jodaka
Copy link
Author

jodaka commented May 6, 2024

I have one project where we (for historical reasons) has been using SASS syntax instead of SCSS. Feature wise both are exactly the same, but SASS syntax is a bit more brief. So I wonder whether zed-scss and underlying tree-sitter has/will support SASS syntax?

Sorry for derailing

@xpe
Copy link

xpe commented May 6, 2024

@bajrangCoder Thanks for posting the issue! For the record it is : tree-sitter-grammars/tree-sitter-scss#4

@xpe
Copy link

xpe commented May 6, 2024

@jodaka and @bajrangCoder To clear some things up as to correct Sass/SCSS syntax:

  1. @mixin is used to define mixins. It belongs at the top-level of a Sass/SCSS file. It is not valid inside a declaration.

  2. @include is the correct way to include a mixin inside a declaration.

References:

@bajrangCoder
Copy link
Owner

@xpe Initially, I was confused because I had never encountered this before. I mistakenly thought it might be a new feature of SCSS. However, upon reflection, I realized that my confusion stemmed from not having worked with SCSS for several months. My apologies for the oversight. 😑

@xpe
Copy link

xpe commented May 6, 2024

@bajrangCoder No worries. In any case, there still is a syntax highlighting problem: nested @include doesn't work. See screenshot:

image

@bajrangCoder
Copy link
Owner

bajrangCoder commented May 6, 2024

@bajrangCoder No worries. In any case, there still is a syntax highlighting problem: nested @include doesn't work. See screenshot:

image

Check the syntax tree for more information (open it from zed command palette), seems this is also not parsed correctly

Currently, I'm away from my PC; otherwise, I would investigate further. Perhaps I'll check back in an hour.

@jodaka
Copy link
Author

jodaka commented May 8, 2024

  1. @mixin is used to define mixins. It belongs at the top-level of a Sass/SCSS file. It is not valid inside a declaration.
  2. @include is the correct way to include a mixin inside a declaration.

At the project I'm supporting at the moment postcss-mixins is used instead of pure scss (basically there are multiple postcss plugins that allow using scss without scss compiler). https://github.com/postcss/postcss-mixins/
And they use @define_mixin for the actual mixin and @mixin to include it. That allows them to separate inclusion of mixins from inclusions of other files.

@xpe, honestly I haven't realised that postcss-mixins syntax is different from original scss until I read your comment. Sorry for confusion.

vscode seem to handle that syntax just fine, so maybe it still worth the effort to support it in zed

@xpe
Copy link

xpe commented May 8, 2024

@jodaka Makes sense, but I'm not quite following this part:

vscode seem to handle that syntax just fine, so maybe it still worth the effort to support it in zed

This might be obvious, but I just want to make sure we're on the same page: the Zed SCSS extension should support only SCSS, right? (PostCSS is an altogether different project with different syntax and different semantics.)

@jodaka
Copy link
Author

jodaka commented May 8, 2024

This might be obvious, but I just want to make sure we're on the same page: the Zed SCSS extension should support only SCSS, right? (PostCSS is an altogether different project with different syntax and different semantics.)

That's not really a question for me, but rather for extension author. As a user of both zed and SCSS I would love one extension that could handle all the SCSS related cases (that is SCSS itself, SASS syntax and postcss variation).

For the sake of keeping issues clean I think this issue might stay open only for nested @include bug. Everything else should be in separate issues.

@xpe
Copy link

xpe commented May 8, 2024

I just want to make sure we're on the same page:

  • PostCSS is not a variation of Sass.
  • SCSS is a syntax of Sass.
  • (The other Sass syntax (the original) is denoted by .sass.)

My point (sorry if this is obvious): if an extension is called "Zed-SCSS" or "zed-scss" it would be confusing to support both SCSS and PostCSS. Better to create a separate extension for PostCSS.

@bajrangCoder bajrangCoder added wontfix This will not be worked on and removed bug Something isn't working labels Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants