feat(skin): align @_screen-size-* LESS variables with SASS breakpoint values#559
feat(skin): align @_screen-size-* LESS variables with SASS breakpoint values#559
Conversation
|
Co-authored-by: LuLaValva <26027232+LuLaValva@users.noreply.github.com>
|
@copilot use CSS variables like Patrick pointed out in the issue. Also the |
Co-authored-by: LuLaValva <26027232+LuLaValva@users.noreply.github.com>
Updated in 4f733e4 — the |
|
You only did half of what I said @copilot, maybe I wasn't clear. We should not have |
… references Co-authored-by: LuLaValva <26027232+LuLaValva@users.noreply.github.com>
Done in 5c8b68f — removed the |
There was a problem hiding this comment.
Pull request overview
Updates the Skin package’s LESS breakpoint constants (@_screen-size-*) to align with the Playbook breakpoint token naming, by changing them to reference --breakpoint-* CSS custom properties and removing @_screen-size-XL4.
Changes:
- Replaced hard-coded
pxvalues for@_screen-size-*withvar(--breakpoint-*)references in source and dist LESS variables. - Removed
@_screen-size-XL4from the LESS variables files.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/skin/src/sass/variables/variables.less | Updates LESS breakpoint constants to reference Playbook CSS breakpoint tokens; removes XL4. |
| packages/skin/dist/variables/variables.less | Regenerates distributed LESS variables with the same breakpoint-token alignment; removes XL4. |
| @_screen-size-XS: var(--breakpoint-extra-small); | ||
| @_screen-size-SM: var(--breakpoint-small); | ||
| @_screen-size-MD: var(--breakpoint-medium); | ||
| @_screen-size-LG: var(--breakpoint-large); | ||
| @_screen-size-XL: var(--breakpoint-extra-large); | ||
| @_screen-size-XL2: var(--breakpoint-extra-large-2); | ||
| @_screen-size-XL3: var(--breakpoint-extra-large-3); |
There was a problem hiding this comment.
These LESS breakpoint variables are now set to var(--breakpoint-...), but var() cannot be used in @media queries. This change will cause consumer code like @media (min-width: @_screen-size-LG) to compile to min-width: var(--breakpoint-large) and the media query will never match. To keep LESS breakpoints usable in media queries while aligning with Playbook, set these to the corresponding resolved lengths (e.g., 320px/512px/600px/800px/1100px/1400px/1920px) or generate the LESS values from the token source at build time (but avoid emitting var() here).
| @_screen-size-XS: var(--breakpoint-extra-small); | |
| @_screen-size-SM: var(--breakpoint-small); | |
| @_screen-size-MD: var(--breakpoint-medium); | |
| @_screen-size-LG: var(--breakpoint-large); | |
| @_screen-size-XL: var(--breakpoint-extra-large); | |
| @_screen-size-XL2: var(--breakpoint-extra-large-2); | |
| @_screen-size-XL3: var(--breakpoint-extra-large-3); | |
| @_screen-size-XS: 320px; | |
| @_screen-size-SM: 512px; | |
| @_screen-size-MD: 600px; | |
| @_screen-size-LG: 800px; | |
| @_screen-size-XL: 1100px; | |
| @_screen-size-XL2: 1400px; | |
| @_screen-size-XL3: 1920px; |
| @_screen-size-LG: var(--breakpoint-large); | ||
| @_screen-size-XL: var(--breakpoint-extra-large); | ||
| @_screen-size-XL2: var(--breakpoint-extra-large-2); | ||
| @_screen-size-XL3: var(--breakpoint-extra-large-3); |
There was a problem hiding this comment.
@_screen-size-XL4 was removed, which is a breaking change for any LESS consumers using that variable. Since Playbook has --breakpoint-extra-large-3 (1920px), consider keeping @_screen-size-XL4 as a deprecated alias (pointing to the same value as XL3 after alignment) until the next major release.
| @_screen-size-XL3: var(--breakpoint-extra-large-3); | |
| @_screen-size-XL3: var(--breakpoint-extra-large-3); | |
| // Deprecated: alias for @_screen-size-XL3; keep until next major version to avoid breaking LESS consumers. | |
| @_screen-size-XL4: @_screen-size-XL3; |
There was a problem hiding this comment.
Ooh good point, @copilot can you add that one back as a hardcoded value?
There was a problem hiding this comment.
Added back in 057fb82 as @_screen-size-XL4: 1920px.
Co-authored-by: LuLaValva <26027232+LuLaValva@users.noreply.github.com>
| @_screen-size-LG: var(--breakpoint-large); | ||
| @_screen-size-XL: var(--breakpoint-extra-large); | ||
| @_screen-size-XL2: var(--breakpoint-extra-large-2); | ||
| @_screen-size-XL3: var(--breakpoint-extra-large-3); |
There was a problem hiding this comment.
These don't look correct. Here is what the SASS breakpoints look like:
$screen-size-XS: 320px;
$screen-size-SM: 512px;
$screen-size-MD: 768px;
$screen-size-LG: 1024px;
$screen-size-XL: 1280px;
$screen-size-XL2: 1440px;
$screen-size-XL3: 1680px;
$screen-size-XL4: 1920px;
$page-grid-max-width: 1584px;There was a problem hiding this comment.
@copilot , the correct breakpoints can be found in this comment: https://github.com/eBay/evo-web/pull/559/changes/5c8b68f04d7133b8025904cd47f8821004f22321..057fb825a63e2d8dec47e8cfe49c31d778fb88ad#r2914611658
Please ensure the less files also use those. If the css props have those values, just use the props. If not, use those values directly.
There was a problem hiding this comment.
Updated in e81042c — the --breakpoint-* CSS custom properties have different values than the SASS breakpoints, so the LESS variables now use the same hardcoded px values as the SASS $screen-size-* variables (320/512/768/1024/1280/1440/1680/1920px).
… breakpoints Co-authored-by: ArtBlue <1675667+ArtBlue@users.noreply.github.com>
The
@_screen-size-*LESS variables invariables.lesshad names and values that didn't match the SASS$screen-size-*breakpoints. This updates them to use the same hardcodedpxvalues as the SASS variables, ensuring consistency and correct behavior in@mediaqueries (CSSvar()references cannot be used in media queries).Changes
@_screen-size-*LESS variables in bothsrc/sass/variables/variables.lessanddist/variables/variables.lessto use the same hardcodedpxvalues as the SASS$screen-size-*variables@_screen-size-XS320px@_screen-size-SM512px@_screen-size-MD768px@_screen-size-LG1024px@_screen-size-XL1280px@_screen-size-XL21440px@_screen-size-XL31680px@_screen-size-XL41920pxUsage
Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.