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

Add support for nested CSS #9

Open
romainmenke opened this issue May 29, 2023 · 26 comments · May be fixed by #30
Open

Add support for nested CSS #9

romainmenke opened this issue May 29, 2023 · 26 comments · May be fixed by #30

Comments

@romainmenke
Copy link
Contributor

romainmenke commented May 29, 2023

At this time nested CSS is not support because at rule and selector patterns aren't included in rules.

Screenshot 2023-05-29 at 23 36 07
@romainmenke
Copy link
Contributor Author

This feature is impossible to add safely without a working test suite for this package.

@radium-v
Copy link

Hi,

My extension Better Less has full syntax highlighting support for nested selectors, and also works with plain CSS. Would it be possible to adapt the work I've done there for this extension?

@romainmenke if could you point me to any similar packages that have a qualifying working test suite that I could use as a guide for implementing it here, I'd be happy to help out.

@romainmenke
Copy link
Contributor Author

Hi @radium-v,

For test discussions I opened this issue : #8
I asked the same question to the maintainers but haven't had any reply yet :/

At this point it is not the work itself that is the issue, but rather knowing that is correct and doesn't introduce regressions in existing features.

So having another extensions that we could adapt bits from does help at some level but it doesn't unblock this work.

@jacobcassidy
Copy link
Contributor

I saw the issues holding back adding CSS Nesting in VSCode so I created the CSS Nesting Syntax Highlighting extension which can be used until it's natively available. We can merge the extension code into vscode-css after testing.

I also added syntax highlighting for@container and functions in at-rules, such as @media (max-width: calc( (768 / 16) * 1rem )) {} into the extension. We would need to create separate PRs for them, of course.

GitHub Repo
VCode Marketplace Page

When I have more time, I'll run the tests mentioned in #8 and submit PRs if the maintainers like added grammar.

@wesbos
Copy link

wesbos commented Sep 17, 2024

with container queries and nesting now in all browsers - is it possible to get some urgency on implementing this? CSS highlighting is kind of broken in VS code right now

@romainmenke
Copy link
Contributor Author

romainmenke commented Sep 17, 2024

Maybe interesting to make this a community funded effort?

I (and maybe others?) could do this work but it is too large to do in my free time in between 9 to 5, doing dishes, ... Having funding would allow me to work on this on the job.

Having good native editor support for CSS is really important for the web, all frontend developers and also has tremendous value for Microsoft as a selling point for VSCode.

I could setup a sponsoring goal on the CSSTools org to gather funds if people are interested in this?

@radium-v
Copy link

I'm currently working on a variant extension called Better CSS, but it's in very early stages (so far at-rules are the only things implemented).

@romainmenke
Copy link
Contributor Author

romainmenke commented Sep 17, 2024

@radium-v It would be nice to see something like this integrated into VSCode itself.
Given that nesting is standard CSS and that VSCode supports standard CSS out of the box.

Ideally users shouldn't need to install an extra extension to be able to write more modern standard CSS :)

Is this something you think that could be back ported into this package?

@radium-v
Copy link

#8 and #23 would go a very long way towards accelerating development here. I've looked at side-porting to get features in here but this grammar file is written in CSON which makes it difficult to contribute. This is why I'm rewriting it as a standalone extension in my personal time.

(For what it's worth, vscode's built-in Less syntax highlighting is now using Better Less, and nested declarations were one of the main reasons why I wrote Better Less in the first place.)

@jacobcassidy jacobcassidy linked a pull request Sep 18, 2024 that will close this issue
@jacobcassidy
Copy link
Contributor

jacobcassidy commented Sep 18, 2024

I submitted a PR that should close this issue (#30).

I currently have limited time, so if anyone else wants to test the updated css.cson file (converted from the CSS Nesting Syntax Highlighting extension's css.tmLanguage.json file), that would help speed up the progress.

@romainmenke
Copy link
Contributor Author

Thank you for this effort @jacobcassidy 🎉

if anyone else wants to test the updated css.cson file

As I've already mentioned here, there currently isn't any test suite for this repo.
That means that any non-trivial changes can't be tested or verified automatically.
It also means that we can't guard against future regressions.

Mutating this repo without first investing in a test suite seems like a bad idea imho.
It would increase complexity and thus the amount of technical debt.

Fixing tests first and then making changes seems like a more sustainable path forwards.


This is why I'm rewriting it as a standalone extension in my personal time.

@radium-v If you can use any help with this?
More than happy to lend a hand!

@jacobcassidy
Copy link
Contributor

@romainmenke Agreed, a test suite would be ideal moving forward.

@tysonmatanich
Copy link

@romainmenke @jacobcassidy Yes adding tests is ideal, however leaving this in its current broken state seems unreasonable.

@wesbos
Copy link

wesbos commented Dec 6, 2024

@romainmenke Do you have an example of another grammar that has a test suite? How big of a task is it? Really want this highlighting to be in VS Code

@radium-v
Copy link

radium-v commented Dec 6, 2024

@wesbos microsoft/Typescript-tmlanguage looks like it has tests: https://github.com/microsoft/TypeScript-TmLanguage/tree/master/tests

@romainmenke
Copy link
Contributor Author

Microsoft devs provided some info here: #8 (comment)

This is a large task, will take dozens of hours.
It also isn't particularly fun work 😅
There were working tests but Microsoft decided to delete all that when they killed Atom.
Rebuilding that for free when an extremely wealthy org caused the current situation is not something I am particularly excited about.

If there is funding for such an effort then this would be just work 😛
I think it is clear by now that Microsoft has very little interest in resolving this :(

@wesbos
Copy link

wesbos commented Dec 6, 2024

Okay so if I understand properly, what needs to happen:

  1. examples of the new CSS Nesting code types need to be added to the fixture here: https://github.com/microsoft/vscode/blob/main/extensions/vscode-colorize-tests/test/colorize-fixtures/test.css

Then running the colorizer tests generates a result snapshot thats available here: https://github.com/microsoft/vscode/blob/main/extensions/vscode-colorize-tests/test/colorize-results/test_css.json

The test only checks that things haven't changed. so theoretically we should just be able to add the new code, run the test, and show that only the new grammars have changed / been added?

@romainmenke
Copy link
Contributor Author

romainmenke commented Dec 6, 2024

Unsure.

My focus has been on these tests: https://github.com/microsoft/vscode-css/blob/main/spec/css-spec.coffee

I don't know if those colorizer tests have sufficient coverage.

@wesbos
Copy link

wesbos commented Dec 9, 2024

I think the idea with the colorizer tests is that it ensures nothing changes from what it's at right now - so I'd say thats probably good enough if that is what they have been using so far?

@romainmenke
Copy link
Contributor Author

I don't think those are anywhere close to full coverage :/
Those are extremely minimal.

Obviously missing aspects are:

  • at rules with blocks
  • complex selectors with functional pseudo's, attributes, ...
  • complex values with various blocks and symbols (e.g. --foo: { "some-json": 1 };)

This is clearly something a lot of people want to see resolved.
Can we focus on getting this resolved in a safe and reliable way?

I really want to avoid causing regressions for the vast majority of CSS authors who don't even use nesting today.

Fixing the tests is doable, adding support for nesting is also doable.
Lets do it the right way :)

Is there anyone willing to volunteer time fixing the tests?
Is anyone willing to sponsor others to fix the tests?

@romainmenke
Copy link
Contributor Author

romainmenke commented Dec 11, 2024

Given that no one seems interested in doing the work, I've taken time off to get this unblocked. I've spend about 4 hours on this so far, which corresponds to roughly €400 in lost wages.

@claviska
Copy link

@romainmenke I found your GitHub sponsors page and chipped in $200 USD. I encourage other folks to do the same.

https://github.com/sponsors/romainmenke

Thanks for contributing to this! 💙

@romainmenke
Copy link
Contributor Author

Thank you so much @claviska for giving back 🙇

@wesbos
Copy link

wesbos commented Dec 11, 2024

Awesome thank you @romainmenke - Syntax just sponsored $400 towards your work. Will see if I can bug some other people/projects as well!

@adamwathan
Copy link

adamwathan commented Dec 11, 2024

@romainmenke Donated $500 from Tailwind CSS too, thanks for all your work!

@romainmenke
Copy link
Contributor Author

Thank you @wesbos and @adamwathan for those extremely generous contributions!

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 a pull request may close this issue.

7 participants