withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
44.1k stars 2.29k forks source link

>=2.3.0 something changed in scss processing #7508

Closed marceloverdijk closed 9 months ago

marceloverdijk commented 1 year ago

What version of astro are you using?

>=2.3.0

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

npm

What operating system are you using?

Mac

What browser are you using?

Chrome

Describe the Bug

I'm using official getbootstrap.com themes in a Astro side by using the scss provided by the theme.

That always worked except when upgrading from 2.2.3 to 2.3.0 after it was released. II notices some strange css behaviour; back then in the nouislider component.

Anyway I thought it was something minor and decided to wait for an upgrade.

I now switched to another getbootstrap.com theme (using `2.2.3) and the theme works as expected.

Decided to update Astro to latest 2.7.1 and I see some unexpected styling issues still unfortunately.

E.g. this how it's rendered with 2.7.1

image

Note the position of the 2nd dropdown, and also the height is different.

What's the expected result?

But with 2.2.3 it is rendered as expected:

image

At the moment I'm unable to provide a reproducible example but if needed I will try to provide something.

I'm wondering if there changed something from 2.3.0 in terms of css/scss processing

My astro.config.mjs looks like:

import { defineConfig } from 'astro/config'

// https://astro.build/config
export default defineConfig({
  site: process.env.NODE_ENV === 'production' ? 'https://www.example.com/' : 'http://localhost:3000/',
  vite: {
    css: {
      preprocessorOptions: {
        scss: {
          quietDeps: true,
        },
      },
    },
  },
})

and postcss.config.js:

module.exports = {
  plugins: [require('autoprefixer')],
}

Link to Minimal Reproducible Example

...

Participation

marceloverdijk commented 1 year ago

Note, despite the Vite sass quiteDeps: true option I still see these warnings:

Deprecation Warning: Using / for division outside of calc() is deprecated and will be removed in Dart Sass 2.0.0.

But I see these warnings for both 2.2.3 and 2.7.1..

bluwy commented 1 year ago

The biggest styling changes in 2.3.0 is https://github.com/withastro/astro/pull/6816, but I'm not sure how that break things. Maybe a path is incorrectly remapped now? It would be hard for us to debug without a repro.

marceloverdijk commented 1 year ago

Thx @bluwy , I will try to create an isolated example repo over the weekend.

marceloverdijk commented 1 year ago

@bluwy I've created an isolated example repo here: https://github.com/marceloverdijk/astro-7508 It uses Astro 2.2.3 which works for me, but any newer version (including latest 2.8.0) seems to give some scss processing issue. If there is anything else I can provide or help with let me know - I'm hoping to upgrade to latest Astro.

bluwy commented 1 year ago

Thanks! I'm able to reproduce it. Reverting https://github.com/withastro/astro/pull/6816 entirely seems to fix it, but I still don't quite understand how that's triggered. Will try to debug this more.

marceloverdijk commented 1 year ago

Thanks for the feedback @bluwy !

marceloverdijk commented 10 months ago

@bluwy any chance changes in upcoming Astro 3.0 release will solve this. I'm currently stuck to old version without possibility to update unfortunately.

bluwy commented 10 months ago

Hey @marceloverdijk. Unfortunately I was heads down getting some 3.0 tasks out, so I haven't get a deeper look into this, and I'll be taking some time off soon.

So the bug might still exist in 3.0, and if so I'll return to this right away after my break! (Or if someone else takes a look at it)

marceloverdijk commented 10 months ago

OK I will try to upgrade to 3.0 anyway and give it a shot. I will report here the findings.

marceloverdijk commented 10 months ago

@bluwy FYI: I've updated the example repo to the latest Astro 3.0.1 but the issue is unfortunately still there.

marceloverdijk commented 9 months ago

Thx @bluwy awesome! this is resolved. Looking forward to the next release so I can upgrade to the latest.

marceloverdijk commented 9 months ago

I can confirm this works with latest 3.1.2 👏 🥳

bluwy commented 9 months ago

Awesome! Thanks for the update.

marceloverdijk commented 9 months ago

Hi @bluwy despite the original issue fixed, I see some other issues now.

With 2.3.1 a page is generated like:

image

but with 3.1.2:

image

See the spacing between the badges in the top left of the cards.

This is producible with the example repo mentioned earlier.

And in the actual app I'm working with I see now more of these small differences. Some of I can reproduce in the example repo, but some not. As far as I could investigate they all seem to be related to css ordering in combination with css variables.

I'm now wondering if I'm doing something wrong in my project setup...

bluwy commented 9 months ago

I can see there's duplicated CSS in 3.0 for some reason, but the styles are still the same and I can't tell what is causing the missing space. In v2 it seems to be coming from the inline block natural spacing, but it's collapsed in 3.0 for some reason.

marceloverdijk commented 9 months ago

Yes I also noticed (partially) duplicated css.

E.g. I have a span like:

<span class="badge bg-primary bg-opacity-10 text-primary">Hotel</span>

and the generated css contains:

L28048:

.bg-primary {
  --bs-bg-opacity: 1;
  background-color: rgba(var(--bs-primary-rgb), var(--bs-bg-opacity)) !important;
}

L28116:

.bg-opacity-10 {
  --bs-bg-opacity: 0.1;
}

L34947:

.bg-primary {
  --bs-bg-opacity: 1;
  background-color: rgba(var(--bs-primary-rgb), var(--bs-bg-opacity)) !important;
}

The .bg-primary is indeed duplicated, but the bg-opacity-10 not. The problem now is that the --bs-bg-opacity is overwritten again.

So the (partially) duplicates css is causing an issue here.

1 option I see is to compile the scss to css myself, and include that file in my project. But not sure if that is the best approach when using Astro...

bluwy commented 9 months ago

I'm seeing the Vite injected styles and Astro injected styles are strangely conflicting too. Would be great if you can open another issue for this 🙏

marceloverdijk commented 9 months ago

Thx for following up @bluwy ; I have created https://github.com/withastro/astro/issues/8637