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

Css Nesting Build Script #7462

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

TheColaber
Copy link
Member

@TheColaber TheColaber commented May 23, 2024

Creates a package.json command npm run transpile-css. When run, it scans all CSS and HTML files and replaces any code that uses nested CSS with plain CSS. This is for compatibility support. Developers will be able to write all CSS using nested CSS if desired and it will still work for the user's devices. Note: This command probably won't be used by developers, it will likely be used when we are sending the extension to web stores.

I made 2 test files, webpages/settings/components/addon-group-header.html and webpages/styles/components/badges.css. Run the package command and the files will get replaced.

I considered using an HTML parser library rather than using regex to find <style> elements but I decided not to since most of the time it alters the HTML especially when it comes to using self-closing tags. We should discuss this further since I'm not really sure of the best route.

Also please keep in mind that I have no idea if this is how I should have done things, but now we have somewhere to start at least :)

@WorldLanguages told me to work on this.

@TheColaber TheColaber added type: enhancement New feature for the project status: pending A PR is still not ready to merge, or an issue is being worked on/on consideration scope: development Related to developing, documentation, DevEx, etc. labels May 23, 2024
.addon-group:hover > img {
background-color: var(--hover-highlight);
}
& > img {
Copy link
Member

Choose a reason for hiding this comment

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

Personally I find it quite confusing to know when the ampersand is needed and when it's not. Since Chrome 120+ and Firefox 117+ it is needed in fewer places, see: https://developer.chrome.com/blog/css-nesting-relaxed-syntax-update

Copy link
Member Author

@TheColaber TheColaber May 23, 2024

Choose a reason for hiding this comment

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

@WorldLanguages Actually I believe you do not need an ampersand in this situation but I put one because I thought it looked better and was more readable. It works with it and without it both in the browser and after being built.

@WorldLanguages
Copy link
Member

If this is merged with a package.json file, we should look for other issues/PRs that suggest this file should exist.

@Samq64
Copy link
Member

Samq64 commented May 23, 2024

#6272 has its own package.json in the the webpages directory and uses npm instead of yarn.

@TheColaber
Copy link
Member Author

#6272 has its own package.json in the the webpages directory and uses npm instead of yarn.

Shouldn't be an issue then. In fact, it would probably make more sense for their pr to move the package.json to the root, regardless, should be fine.

@TheColaber TheColaber requested a review from mxmou May 23, 2024 18:13
@WorldLanguages
Copy link
Member

I mean, mxmou doesn't need to review the build script, but I'm wondering if it's worth exploring this, as this may simplify writing CSS.
I just checked the original scratchr2.user.css and it does not use nesting, though. I don't know if that's intentional or if the preprocessor didn't support it.

@DNin01
Copy link
Member

DNin01 commented May 24, 2024

If this is merged with a package.json file, we should look for other issues/PRs that suggest this file should exist.

I have a related task in the checklist of #6721.

@Hans5958
Copy link
Member

Hans5958 commented May 24, 2024

I prefer NPM (package-lock.json) over Yarn (yarn.lock), since that's what we used around here (ScratchAddons/contributors, ScratchAddons/website-v2-script, ScratchAddons/l10n-script).

One step closer to build step...

@mxmou
Copy link
Member

mxmou commented May 27, 2024

I just checked the original scratchr2.user.css and it does not use nesting, though. I don't know if that's intentional or if the preprocessor didn't support it.

I don't remember why I didn't use nesting - probably because I was copying selectors from Scratch's styles. Nested selectors would certainly be useful.

@GrahamSH-LLK
Copy link
Member

This should be be able to be solved within vite fairly easily (eg. using built in lightningcss support)
I can add it to my PR?

@Hans5958
Copy link
Member

Hans5958 commented Jun 4, 2024

This should be be able to be solved within vite fairly easily (eg. using built in lightningcss support) I can add it to my PR?

Would be nice, but then I don't think your PR will be merged in the near future.

I would say we can do this first then replace it later if needed.

@GrahamSH-LLK
Copy link
Member

This should be be able to be solved within vite fairly easily (eg. using built in lightningcss support) I can add it to my PR?

Would be nice, but then I don't think your PR will be merged in the near future.

I would say we can do this first then replace it later if needed.

well my PR works quite well and really could be merged 😛

obviously we need to get mv3 done before anything else but i really do think it's ready to merge.

@TheColaber TheColaber removed the status: pending A PR is still not ready to merge, or an issue is being worked on/on consideration label Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: development Related to developing, documentation, DevEx, etc. type: enhancement New feature for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants