locomotivemtl / locomotive-boilerplate

🚂 Front-end boilerplate for projects by Locomotive.
https://locomotivemtl-boilerplate.vercel.app/
MIT License
455 stars 71 forks source link

Create aspect-ratio SCSS mixin polyfill #156

Closed lucasvallenet closed 7 months ago

lucasvallenet commented 11 months ago

Create an aspect-ratio mixins with a fallback for unsupported browsers.

Is it relevant considering the current support ? I dont know :)

cloudflare-pages[bot] commented 10 months ago

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 12d65db
Status: âœ…  Deploy successful!
Preview URL: https://adde1d33.locomotive-boilerplate.pages.dev
Branch Preview URL: https://feature-sass-aspect-ratio.locomotive-boilerplate.pages.dev

View logs

butterfail commented 7 months ago

Hello @lucasvallenet @mcaskill,

I've noticed a potential issue in the implementation of the aspect-ratio mixin. The @supports not (aspect-ratio: 1) and @supports (aspect-ratio: 1) directives appear to be reversed. As it stands, the code applies styles intended for browsers that do not support the aspect-ratio property in cases where it is actually supported, and vice versa.

Here is the concerned part:

@supports not (aspect-ratio: 1) {
    aspect-ratio: $ratio;
}

@supports (aspect-ratio: 1) {
   height: 0;
   padding-top: calc(#{$width} * #{math.div(1, $ratio)});
   [...]
}

I suggest reversing these two blocks to ensure that the appropriate styles are applied based on whether the browser supports the aspect-ratio property or not.

@supports (aspect-ratio: 1) {
    aspect-ratio: $ratio;
}

@supports not (aspect-ratio: 1) {
   height: 0;
   padding-top: calc(#{$width} * #{math.div(1, $ratio)});
   [...]
}
lucasvallenet commented 7 months ago

Hello @GregoireCiles,

You are right, it is now fixed Thank you!