Skip to content

feat(skin): align @_screen-size-* LESS variables with Playbook breakpoint values #559

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.

Sign up for GitHub

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

Open
Copilot wants to merge 7 commits into
base: main
Choose a base branch
from
+10 −10
Open

feat(skin): align @_screen-size-* LESS variables with Playbook breakpoint values #559

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
18ce8c7
Initial plan
Copilot Mar 10, 2026
ebc0020
feat(skin): add Playbook-aligned breakpoint LESS variables
Copilot Mar 10, 2026
4f733e4
feat(skin): use CSS variable references for breakpoint LESS variables
Copilot Mar 10, 2026
5c8b68f
feat(skin): update @_screen-size-* LESS variables to use CSS variable…
Copilot Mar 10, 2026
057fb82
feat(skin): restore @_screen-size-XL4 as hardcoded 1920px value
Copilot Mar 10, 2026
e81042c
feat(skin): use hardcoded px values for @_screen-size-* to match SASS…
Copilot Mar 16, 2026
8efb2af
feat(skin): update @_screen-size-* LESS variables to Playbook-aligned…
Copilot Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions packages/skin/dist/variables/variables.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@
// most mobile devices would be higher in screen width than the smallest viewport width.

// The variables with underscores are to be considered constants. They can be read, but should not be modified.
@_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;
@_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);

@_page-grid-max-width: 1584px;
@_page-grid-number-cols-small: 8;
Expand Down
15 changes: 7 additions & 8 deletions packages/skin/src/sass/variables/variables.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@
// most mobile devices would be higher in screen width than the smallest viewport width.

// The variables with underscores are to be considered constants. They can be read, but should not be modified.
@_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;
@_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);
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
@_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;

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

@_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.

Suggested change
@_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;

Copilot uses AI. Check for mistakes.
Copy link
Member

@LuLaValva LuLaValva Mar 10, 2026

Choose a reason for hiding this comment

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

Ooh good point, @copilot can you add that one back as a hardcoded value?

Copy link
Contributor Author

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

Added back in 057fb82 as @_screen-size-XL4: 1920px.

Copy link
Contributor

@ArtBlue ArtBlue Mar 10, 2026

Choose a reason for hiding this comment

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

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;

Copy link
Contributor

@ArtBlue ArtBlue Mar 16, 2026

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Copilot AI Mar 16, 2026

Choose a reason for hiding this comment

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

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).


@_page-grid-max-width: 1584px;
@_page-grid-number-cols-small: 8;
Expand Down