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

chore(responsive): add xxs breakpoint prop #8041

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

jcfranco
Copy link
Member

Related Issue: #7856

Summary

Updates breakpoints lookup object to include xxs (added in #7992).

@jcfranco
Copy link
Member Author

cc @driskull

@jcfranco jcfranco added the skip visual snapshots Pull requests that do not need visual regression testing. label Oct 23, 2023
@github-actions github-actions bot added the chore Issues with changes that don't modify src or test files. label Oct 23, 2023
@jcfranco jcfranco added the low risk Issues with low risk for consideration in low risk milestones label Oct 23, 2023
--calcite-app-breakpoint-width-md: 1000px;
--calcite-app-breakpoint-width-sm: 100px;
--calcite-app-breakpoint-width-xs: 10px;
--calcite-app-breakpoint-width-xxs: 1px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Breakpoints as CSS Custom Props don't actually work. They can be queried by JS but this is not the ideal way to retrieve these variables. These tokens are also provided by calcite-design-tokens as CSS custom props. However the values do not align.

--calcite-app-breakpoint-width-lg: 1440px; /* min-width size */
  --calcite-app-breakpoint-width-md: 1152px; /* min-width size */
  --calcite-app-breakpoint-width-sm: 768px; /* min-width size */
  --calcite-app-breakpoint-width-xs: 476px; /* min-width size */
  --calcite-app-breakpoint-width-xxs: 320px;

Or as SCSS variables

$breakpoint-width-lg: 1440px; // min-width size
$breakpoint-width-md: 1152px; // min-width size
$breakpoint-width-sm: 768px; // min-width size
$breakpoint-width-xs: 476px; // min-width size
$breakpoint-width-xxs: 320px; // Small handheld devices and mini-windows

The new PR (8044) also provides the same breakpoints now as a JS object

import tokenJSON from "@esri/calcite-design-tokens/js";

const breakpointXxs = tokenJSON.core.breakpoint.width.xxs.value;

or as tree-shakeable named exports

import { CoreBreakpointWidthXxs } from "@esri/calcite-design-tokens/es6";

Do we still need this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Breakpoints as CSS Custom Props don't actually work.

Can you provide more info on this? I'm not sure I follow.

They can be queried by JS but this is not the ideal way to retrieve these variables. These tokens are also provided by calcite-design-tokens as CSS custom props. However the values do not align.

There are a few factors on why we get the breakpoints by evaluating the CSS at runtime:

  1. no JS object at the time
  2. unclear if we were allowing users to override breakpoints via CSS props or not
  3. wanted to avoid introducing a third way to reference tokens within the code base

Depending on 2, we could revisit this module's implementation (in a follow-up PR) and set some guidelines for 3.

These breakpoints do not align with designer approved breakpoints

FWIW, this test cover looking up the breakpoints CSS props and isn't meant to match the actual breakpoint values.

Copy link
Contributor

@alisonailea alisonailea Oct 24, 2023

Choose a reason for hiding this comment

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

Can you provide more info on this? I'm not sure I follow.

https://bholmes.dev/blog/alternative-to-css-variable-media-queries/

  1. unclear if we were allowing users to override breakpoints via CSS props or not

While a consumer could override any custom prop including --calcite-app-breakpoint-width-lg it won't do them any good because CSS will not use custom props in media breakpoint queries as shown in the link above. This can be accomplished via SCSS but that would rely on the SCSS var $breakpoint-width-lg before compiling.

FWIW, this test cover looking up the breakpoints CSS props and isn't meant to match the actual breakpoint values.

Got it! Thanks for the clarification.

Copy link
Contributor

Choose a reason for hiding this comment

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

Container Style Queries can be used to set breakpoint styles with CSS Custom Props but that's currently only available in chrome https://developer.chrome.com/blog/style-queries/

Copy link
Member Author

Choose a reason for hiding this comment

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

While a consumer could override any custom prop including --calcite-app-breakpoint-width-lg it won't do them any good because CSS will not use custom props in media breakpoint queries as shown in the link above. This can be accomplished via SCSS but that would rely on the SCSS var $breakpoint-width-lg before compiling.
Container Style Queries can be used to set breakpoint styles with CSS Custom Props but that's currently only available in chrome https://developer.chrome.com/blog/style-queries/

Ah, that's right. Thanks for catching that and clarifying.

The responsive util does pick up breakpoint CSS props overrides at load time, but we would still have a disconnect in a few places (namely stylesheet and conditional rendering) because we can't use style queries/environment variables yet.

I'll update the util in a follow-up PR to use the JS exports instead. ✨🔧✨

@alisonailea alisonailea self-requested a review October 24, 2023 16:15
Copy link
Contributor

@alisonailea alisonailea left a comment

Choose a reason for hiding this comment

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

These breakpoints do not align with designer approved breakpoints

My bad this is just for testing.

@jcfranco jcfranco merged commit f7ecc6c into main Oct 25, 2023
12 of 14 checks passed
@jcfranco jcfranco deleted the jcfranco/add-xxs-breakpoint-prop branch October 25, 2023 05:03
jcfranco added a commit that referenced this pull request Nov 10, 2023
…ct (#8051)

**Related Issue:** N/A

## Summary

Stems from
#8041 (comment).

This changes how breakpoints are built and used (`responsive` module):

* uses design tokens JS output target to reference breakpoint values
* prevents possible disconnect between stylesheet and conditional
rendering breakpoint values (only the latter could be overridden via CSS
props)
* the breakpoints lookup object no longer needs to be obtained
asynchronously

This also updates and unskips the `responsive` spec test.

**Note**: this includes an update to the testing Jest config to apply
Stencil's transformer to the design token's headless ESM file as the
default one was not transforming properly and leading to `Jest
encountered an unexpected token` errors when running the spec test.
benelan pushed a commit that referenced this pull request Nov 13, 2023
…ct (#8051)

**Related Issue:** N/A

## Summary

Stems from
#8041 (comment).

This changes how breakpoints are built and used (`responsive` module):

* uses design tokens JS output target to reference breakpoint values
* prevents possible disconnect between stylesheet and conditional
rendering breakpoint values (only the latter could be overridden via CSS
props)
* the breakpoints lookup object no longer needs to be obtained
asynchronously

This also updates and unskips the `responsive` spec test.

**Note**: this includes an update to the testing Jest config to apply
Stencil's transformer to the design token's headless ESM file as the
default one was not transforming properly and leading to `Jest
encountered an unexpected token` errors when running the spec test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issues with changes that don't modify src or test files. low risk Issues with low risk for consideration in low risk milestones skip visual snapshots Pull requests that do not need visual regression testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants