bajrangCoder / zed-scss

SCSS support for Zed
9 stars 1 forks source link

Bug in highlighting #2

Open jodaka opened 2 months ago

jodaka commented 2 months ago

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 commented 2 months ago

Really that's not good highlighting.

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

bajrangCoder commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

jodaka commented 2 months ago

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 commented 2 months ago

@bajrangCoder Thanks for posting the issue! For the record it is : https://github.com/tree-sitter-grammars/tree-sitter-scss/issues/4

xpe commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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

image
bajrangCoder commented 2 months ago

@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 commented 2 months ago
  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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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

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.