fix(color-context tokens): only apply when `.calcite-mode-auto` is applied #8344

alisonailea · 2023-12-06T03:09:11Z

Related Issue: #8343

Summary

Resolves bug where the @media query, which applied light and dark color-context tokens on prefers-color-scheme in CSS, was applied at :root instead of .calcite-mode-auto

benelan

Did you do testing to confirm that it no longer uses dark mode if prefer-color-scheme is "dark"? If I remember correctly, calcite-mode-auto uses the value from prefer-color-scheme, so this change may end up with the same outcome.

I think we don't want to add any of the mode classes by default, which defaults to the components to "light".

ref: https://developers.arcgis.com/calcite-design-system/core-concepts/#modes

benelan · 2023-12-06T03:29:26Z

...design-tokens/support/token-transformer/styleDictionary/formatter/utils/formatExtraOutput.ts

@@ -44,7 +44,7 @@ export function formatExtraOutput(
            ? index.media.map(([mediaSchemed, output]) => {
                const m = ensureIfArray(outputObject[`${output}.${args.platform}`]);
                return Array.isArray(m)
-                  ? `@media (${mediaSchemed}) {\n\t:root {\n\t\t${m
+                  ? `@media (${mediaSchemed}) {\n\t.calcite-mode-auto {\n\t\t${m


As a follow up (not in this PR), we shouldn't be using \n since it can cause issues on Windows. Is there an issue for tracking the Windows related design token build errors that Kitty ran into? If not, can you please log one?

For reference, you can do this instead:

import { EOL } from "os"; const osAgnosticString = `${EOL}lorem${EOL}ipsum${EOL}`

driskull

Nice 👍

alisonailea · 2023-12-06T04:28:28Z

Did you do testing to confirm that it no longer uses dark mode if prefer-color-scheme is "dark"? If I remember correctly, calcite-mode-auto uses the value from prefer-color-scheme, so this change may end up with the same outcome.

The e2e tests caught that light mode was not being applied by default.

...design-tokens/support/token-transformer/styleDictionary/formatter/utils/formatExtraOutput.ts

benelan

Couple more nitpicks, but otherwise code LGTM! Thanks for the quick turn around!

Just to confirm, the .calcite-mode-auto class isn't being added by default right?

It's fine for this PR, but in the future can we split the mode fix and the EOL refactor into two separate PRs? It makes it easier to determine what actually resolved the bug, as well as easier to revert one w/o losing the other's work if a regression occurs.

Stepping back a bit, would it make more sense to run the transformed strings/files through Prettier's API instead of doing all the tab/newline formatting manually?

benelan · 2023-12-06T04:50:00Z

packages/calcite-design-tokens/support/token-transformer/styleDictionary/formatter/utils.ts

    suffix: ";",
  },
  scss: {
    prefix: "$",
    commentStyle: "short",
    indentation: "",
    separator: ":",
-    join: "\n",
+    join: `${EOL}`,


Nitpick: this can be:

// ... join: EOL, // ...

There are a few other places too, try find/replacing:
`${EOL}` --> EOL

benelan · 2023-12-06T04:55:06Z

...design-tokens/support/token-transformer/styleDictionary/formatter/utils/formatExtraOutput.ts

          } else {
-            writeFileSync(absoluteFilePath, `${args.header}${outputList.join("\n\n")}\n`);
+            writeFileSync(absoluteFilePath, `${args.header}${outputList.join(`${EOL}`)}${EOL}`);


Was this one supposed to be joining on 2 new lines?

alisonailea · 2023-12-06T05:40:34Z

Just to confirm, the .calcite-mode-auto class isn't being added by default right?

No. It is provided as an option. Light mode tokens are set by default.

…ied and default to light

...design-tokens/support/token-transformer/styleDictionary/formatter/utils/formatExtraOutput.ts

jcfranco

Noice! 🚀

jcfranco · 2023-12-06T06:51:17Z

Stepping back a bit, would it make more sense to run the transformed strings/files through Prettier's API instead of doing all the tab/newline formatting manually?

Agreed. Let's do this as a follow-up.

alisonailea · 2023-12-06T17:52:28Z

It's fine for this PR, but in the future can we split the mode fix and the EOL refactor into two separate PRs? It makes it easier to determine what actually resolved the bug, as well as easier to revert one w/o losing the other's work if a regression occurs.

Good point. Refactoring the branch to remove EOL commits and moving those to a separate PR.

…ssage * origin/main: (22 commits) build(deps): update dependency @types/lodash-es to v4.17.12 (#8347) build(deps): update dependency @types/semver to v7.5.6 (#8348) ci: stop formatting components.d.ts (#8358) build(deps): update dependency @types/shell-quote to v1.7.5 (#8349) ci: remove workarounds no longer needed with Stencil v4 (#8325) refactor(input, input-number, input-text): avoid redundant `:host` (#8163) fix(color-context tokens): only apply when `.calcite-mode-auto` is applied (#8344) test(dropdown): fix dropdown E2E suite grouping (#8319) build(deps): update dependency @types/jest-axe to v3.5.9 (#8346) refactor(dropdown): remove obsolete logic (#8321) fix(action-menu): fix closing action menu after a drag occurs (#8339) fix(shell, shell-panel): support resizing shell panel when there is an iframe slotted in shell content (#8317) chore(dropdown): update demo page to use selected vs active (removed) (#8322) fix(action-menu): Filter hidden or disabled actions via keyboard. (#8336) build(deps): update dependency @esri/calcite-ui-icons to v3.25.1 (#8331) build(deps): update dependency @types/color to v3.0.6 (#8332) build(deps): update dependency @types/dedent to v0.7.2 (#8334) build(deps): update dependency @types/estree to v1.0.5 (#8335) chore(demos): update outdated CSS variable names after token breaking changes (#8333) build(deps): update dependency @tokens-studio/sd-transforms to v0.12.1 (#8146) ...

🤖 I have created a release *beep* *boop* --- <details><summary>@esri/calcite-design-tokens: 2.1.0</summary> ## [2.1.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2023-12-19) ### Features * **font tokens:** Add font family fallbacks ([#8389](#8389)) ([b2fd255](b2fd255)) * Update tokens for better documentation ([#8395](#8395)) ([ff19630](ff19630)) ### Bug Fixes * **color tokens:** Fix errors in schema ([#8446](#8446)) ([f36a90e](f36a90e)) * **color-context tokens:** Only apply when `.calcite-mode-auto` is applied ([#8344](#8344)) ([19de817](19de817)) * Replace "\n" to support Windows for tokens output ([#8352](#8352)) ([02cf5d5](02cf5d5)) </details> <details><summary>@esri/calcite-components: 2.1.0</summary> ## [2.1.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2023-12-19) ### Features * Add `validation-message` and `validation-icon` attributes to form components for new internal `calcite-input-message` ([#8305](#8305)) ([a554cfd](a554cfd)) * **dropdown-item:** Add disabled support ([#8312](#8312)) ([4c311c6](4c311c6)) * **list-item:** Add calciteListItemToggle event. ([#8433](#8433)) ([1d2fa04](1d2fa04)) * **list:** Add `calciteListDragStart` and `calciteListDragEnd` events for when a drag starts and ends. ([#8361](#8361)) ([1314605](1314605)) * **list:** Add drag event details to `calciteListDragStart` and `calciteListDragEnd` events ([#8438](#8438)) ([e199c08](e199c08)) * Provide legacy CSS custom props for backwards compatibility ([#8355](#8355)) ([b0f063e](b0f063e)) ### Bug Fixes * **action-menu:** Filter hidden or disabled actions via keyboard. ([#8336](#8336)) ([11c0007](11c0007)) * **action-menu:** Fix closing action menu after a drag occurs ([#8339](#8339)) ([dcb8548](dcb8548)) * **action-menu:** Keep internal popover open property in sync ([#8387](#8387)) ([38dff7c](38dff7c)) * **dropdown-item:** Avoid hover/active styling when disabled ([#8398](#8398)) ([35817dc](35817dc)) * **floating-ui:** Improve floating element performance ([#8409](#8409)) ([4d8cfb8](4d8cfb8)) * **input-date-picker:** Ensure range input toggling is consistent ([#8414](#8414)) ([cd92586](cd92586)) * **input-date-picker:** No longer emits redundant change event ([#8341](#8341)) ([cd5b92b](cd5b92b)) * **input-date-picker:** Respect the numberingSystem property when rendering the input ([#8383](#8383)) ([395b538](395b538)) * **list-item:** Drag grid cell should be accessible via arrow keys. ([#8353](#8353)) ([2718ab3](2718ab3)) * **menu-item:** Improve keyboard navigability when `href` populated ([#8408](#8408)) ([5b44798](5b44798)) * **modal:** Ensure document overflow styles are properly restored when multiple modals are closed/removed ([#8390](#8390)) ([f2c6b09](f2c6b09)) * Replace "\n" to support Windows for tokens output ([#8352](#8352)) ([02cf5d5](02cf5d5)) * **shell-panel:** Adds border at the start when slotted in `panel-end` ([#8314](#8314)) ([2d1a1e2](2d1a1e2)) * **shell, shell-panel:** Support resizing shell panel when there is an iframe slotted in shell content ([#8317](#8317)) ([e0f69c9](e0f69c9)) * **stepper:** Emits `calciteStepperItemChange` event when switched to first step ([#8422](#8422)) ([508979f](508979f)) * **table-cell:** Fix background css variable ([#8439](#8439)) ([9e5c59b](9e5c59b)) * **tab:** Prevent vertical scrollbar on content pane when the height of outer elements are specified ([#8399](#8399)) ([9e6d901](9e6d901)) * Use Stencil watchers instead of global attributes util ([#8407](#8407)) ([c531d81](c531d81)) ### Dependencies * The following workspace dependencies were updated * devDependencies * @esri/calcite-design-tokens bumped from ^2.1.0-next.1 to ^2.1.0 </details> <details><summary>@esri/calcite-components-angular: 2.1.0</summary> ## [2.1.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2023-12-19) ### Miscellaneous Chores * **@esri/calcite-components-angular:** Synchronize undefined versions ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.1.0-next.12 to ^2.1.0 </details> <details><summary>@esri/calcite-components-react: 2.1.0</summary> ## [2.1.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2023-12-19) ### Miscellaneous Chores * **@esri/calcite-components-react:** Synchronize undefined versions ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.1.0-next.12 to ^2.1.0 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

alisonailea added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. p - high Issue should be addressed in the current milestone, impacts component or core functionality labels Dec 6, 2023

alisonailea added this to the 2023 December Priorities milestone Dec 6, 2023

alisonailea self-assigned this Dec 6, 2023

alisonailea requested review from benelan, jcfranco, driskull and geospatialem as code owners December 6, 2023 03:09

benelan reviewed Dec 6, 2023

View reviewed changes

driskull approved these changes Dec 6, 2023

View reviewed changes

github-advanced-security bot found potential problems Dec 6, 2023

View reviewed changes

benelan approved these changes Dec 6, 2023

View reviewed changes

fix(color context tokens): only apply when .calcite-mode-auto is appl…

25e7f9b

…ied and default to light

alisonailea force-pushed the astump/8343-fix-prefers-color-scheme branch from 38408cf to 25e7f9b Compare December 6, 2023 05:44

github-advanced-security bot found potential problems Dec 6, 2023

View reviewed changes

...design-tokens/support/token-transformer/styleDictionary/formatter/utils/formatExtraOutput.ts Fixed Show fixed Hide fixed

jcfranco approved these changes Dec 6, 2023

View reviewed changes

fix: git code scanning issue

48ecd08

alisonailea mentioned this pull request Dec 6, 2023

'\n' in token output files causing issues in Windows #8350

Closed

1 task

alisonailea added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Dec 6, 2023

Merge branch 'main' into astump/8343-fix-prefers-color-scheme

0d0630f

alisonailea added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Dec 6, 2023

alisonailea merged commit 19de817 into main Dec 6, 2023
16 checks passed

alisonailea deleted the astump/8343-fix-prefers-color-scheme branch December 6, 2023 20:30

calcite-admin mentioned this pull request Dec 6, 2023

chore: release main #8329

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(color-context tokens): only apply when `.calcite-mode-auto` is applied #8344

fix(color-context tokens): only apply when `.calcite-mode-auto` is applied #8344

alisonailea commented Dec 6, 2023

benelan left a comment

benelan Dec 6, 2023 •

edited

Loading

driskull left a comment

alisonailea commented Dec 6, 2023

benelan left a comment

benelan Dec 6, 2023

benelan Dec 6, 2023

alisonailea Dec 6, 2023

alisonailea commented Dec 6, 2023

jcfranco left a comment

jcfranco commented Dec 6, 2023

alisonailea commented Dec 6, 2023

fix(color-context tokens): only apply when .calcite-mode-auto is applied #8344

fix(color-context tokens): only apply when .calcite-mode-auto is applied #8344

Conversation

alisonailea commented Dec 6, 2023

Summary

benelan left a comment

Choose a reason for hiding this comment

benelan Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

driskull left a comment

Choose a reason for hiding this comment

alisonailea commented Dec 6, 2023

benelan left a comment

Choose a reason for hiding this comment

benelan Dec 6, 2023

Choose a reason for hiding this comment

benelan Dec 6, 2023

Choose a reason for hiding this comment

alisonailea Dec 6, 2023

Choose a reason for hiding this comment

alisonailea commented Dec 6, 2023

jcfranco left a comment

Choose a reason for hiding this comment

jcfranco commented Dec 6, 2023

alisonailea commented Dec 6, 2023

fix(color-context tokens): only apply when `.calcite-mode-auto` is applied #8344

fix(color-context tokens): only apply when `.calcite-mode-auto` is applied #8344

benelan Dec 6, 2023 •

edited

Loading