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

FIDEFE-4631 - Output tokens in pre-defined order #12

Merged
merged 5 commits into from
Feb 21, 2024

Conversation

mstriemer
Copy link
Collaborator

This defines an order and labels for tokens to be output in. Likely want to move that config somewhere else. We could potentially put it in the tokens but it would be way more verbose when writing those.

I put all the changes at the bottom of the file just to avoid conflicts. I rebased this onto #9 locally and the token output was pretty close to what we have currently 🙌

@mstriemer
Copy link
Collaborator Author

Here's the output for tokens-shared.css currently. Didn't include it in the commit since it was causing conflicts when rebasing onto #9

/**
 * This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/.
 */

/**
 * Do not edit this file directly, instead modify design-tokens.json
 * and run `npm run build` to see your changes.
 */

:root,
:host(.anonymous-content-host) {
  /* Base tokens */
  /** Color **/
  --color-black-a10: rgba(0, 0, 0, 0.1);
  --color-blue-05: #deeafc;
  --color-blue-30: #73a7f3;
  --color-blue-50: #0060df;
  --color-blue-60: #0250bb;
  --color-blue-70: #054096;
  --color-blue-80: #003070;
  --color-cyan-20: #aaf2ff;
  --color-cyan-30: #aaf2ff;
  --color-cyan-50: #00ddff;
  --color-gray-05: #fbfbfe;
  --color-gray-50: #bfbfc9;
  --color-gray-60: #8f8f9d;
  --color-gray-70: #5b5b66;
  --color-gray-80: #23222b;
  --color-gray-90: #1c1b22;
  --color-gray-100: #15141a;
  --color-green-05: #d8eedc;
  --color-green-30: #4dbc87;
  --color-green-50: #017a40;
  --color-green-80: #004725;
  --color-red-05: #ffe8e8;
  --color-red-30: #f37f98;
  --color-red-50: #d7264c;
  --color-red-80: #690f22;
  --color-white: #ffffff;
  --color-yellow-05: #ffebcd;
  --color-yellow-30: #e49c49;
  --color-yellow-50: #cd411e;
  --color-yellow-80: #5a3100;

  /* Application tokens */
  /** Border **/
  --border-radius-circle: 9999px;
  --border-radius-medium: 8px;
  --border-radius-small: 4px;
  --border-width: 1px;

  /** Color **/
  --color-background-critical: light-dark(var(--color-red-05), var(--color-red-80));
  --color-background-information: light-dark(var(--color-blue-05), var(--color-blue-80));
  --color-background-success: light-dark(var(--color-green-05), var(--color-yellow-80));
  --color-background-warning: light-dark(var(--color-yellow-05), var(--color-blue-80));

  /** Text **/
  --text-color-deemphasized: color-mix(in srgb, currentColor 60%, transparent);

  @media (prefers-contrast) {
    /* Application tokens */
    /** Border **/
    --border-color: var(--text-color);
    --border-interactive-color-active: AccentColor;
    --border-interactive-color: AccentColor;
    --border-interactive-color-disabled: GrayText;
    --border-interactive-color-hover: SelectedItem;

    /** Text **/
    --text-color: CanvasText;
    --text-color-deemphasized: inherit;
  }

  @media (forced-colors) {
    /* Application tokens */
    /** Border **/
    --border-interactive-color-active: ButtonText;
    --border-interactive-color: ButtonText;
    --border-interactive-color-disabled: GrayText;
    --border-interactive-color-hover: ButtonText;
  }
}

@mstriemer mstriemer requested review from hannajones, TGiles and jsimplicio and removed request for hannajones and TGiles February 13, 2024 17:21
Copy link

@TGiles TGiles left a comment

Choose a reason for hiding this comment

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

One minor change, but otherwise the output looks good to me!

toolkit/themes/shared/design-system/config.js Show resolved Hide resolved
Copy link

@hannajones hannajones left a comment

Choose a reason for hiding this comment

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

We should commit the built CSS files with the comments, otherwise this looks great!

One suggestion, take it or leave it (also scrutinize it bc my brain might still be on vacation...)

Comment on lines 285 to 311
function sortByName(a, b) {
let aParts = a.name.split("-");
let bParts = b.name.split("-");
if (aParts.length == bParts.length) {
// 05 > 10 when comparing strings, so handle numbers as ints.
for (let i = 0; i < aParts.length; i++) {
let aPart = aParts[i];
let bPart = bParts[i];
if (/^\d+$/.test(aPart) && /^\d+$/.test(bPart)) {
if (parseInt(bPart, 10) > parseInt(aPart, 10)) {
return -1;
}
if (parseInt(aPart, 10) > parseInt(bPart, 10)) {
return 1;
}
}
if (bPart > aPart) {
return -1;
}
if (aPart > bPart) {
return 1;
}
}
return 1;
}
return b.name > a.name ? -1 : 1;
}

Choose a reason for hiding this comment

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

Suggested change
function sortByName(a, b) {
let aParts = a.name.split("-");
let bParts = b.name.split("-");
if (aParts.length == bParts.length) {
// 05 > 10 when comparing strings, so handle numbers as ints.
for (let i = 0; i < aParts.length; i++) {
let aPart = aParts[i];
let bPart = bParts[i];
if (/^\d+$/.test(aPart) && /^\d+$/.test(bPart)) {
if (parseInt(bPart, 10) > parseInt(aPart, 10)) {
return -1;
}
if (parseInt(aPart, 10) > parseInt(bPart, 10)) {
return 1;
}
}
if (bPart > aPart) {
return -1;
}
if (aPart > bPart) {
return 1;
}
}
return 1;
}
return b.name > a.name ? -1 : 1;
}
function sortByName(a, b) {
let aParts = a.name.split("-");
let bParts = b.name.split("-");
if (aParts.length == bParts.length) {
for (let i = 0; i < aParts.length; i++) {
let aPart = aParts[i];
let bPart = bParts[i];
if (aPart !== bPart) {
return aPart.localeCompare(bPart, undefined, { numeric: true });
}
}
return 1;
}
return a.name.localeCompare(b.name);
}

I think this could be simplified a bit by using localeCompare which has an option to handle comparing numbers. Not 100% sure this is right, but something along these lines 😆

Comment on lines 67 to 70
--border-interactive-color-active: AccentColor;
--border-interactive-color: AccentColor;
--border-interactive-color-disabled: GrayText;
--border-interactive-color-hover: SelectedItem;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The sorting of these seems off? But the sorting function seems to give expected results 🤔

> "border-interactive-color".localeCompare("border-interactive-color-active", undefined, { numeric: true })
-1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahha, it still has -base

border-interactive-color-active border-interactive-color-base -1

@TGiles TGiles force-pushed the json-design-tokens branch from 7a630ae to 6ad4ac2 Compare February 15, 2024 22:22
Copy link

@hannajones hannajones left a comment

Choose a reason for hiding this comment

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

Looks good! r+ with the small comments/questions addressed

"Application tokens/Link": "link",
"Application tokens/Text": "text",
"Application tokens/Size": "size",
"Application tokens/Spacing": ["space", "spacing"],

Choose a reason for hiding this comment

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

Suggested change
"Application tokens/Spacing": ["space", "spacing"],
"Application tokens/Space": "space"

Should we just commit to using space? I don't see any spacing tokens currently and we could be consistent and stick to nouns. Also looks like that's what we're using on the current central.

@@ -222,6 +244,71 @@ const createLightDarkTransform = surface => {
return name;
};

function formatVariables({ format, dictionary, outputReferences, formatting }) {

Choose a reason for hiding this comment

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

Can we add a JSDoc comment here? Just to summarize what this is doing + be consistent with the other functions in the config

@mstriemer mstriemer merged this pull request into FirefoxUX:json-design-tokens Feb 21, 2024
@mstriemer mstriemer deleted the format-token-output branch February 21, 2024 21:54
TGiles pushed a commit that referenced this pull request Mar 5, 2024
Sort tokens when exporting in CSS and add section headers to match existing tokens-*.css files in mozilla-central
TGiles pushed a commit that referenced this pull request Mar 8, 2024
Sort tokens when exporting in CSS and add section headers to match existing tokens-*.css files in mozilla-central
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants