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

feat: resolves the design token values [ALT-171] #195

Merged
merged 1 commit into from
Dec 14, 2023
Merged

Conversation

ryunsong-contentful
Copy link
Contributor

Approach

Only do this for margin and padding so we can first agree on the implementation structure and then once agreed we can apply it for all other tokens like direction, background color, border.

Additionally, the strategy is as follows
Use a set to determine which potential variables (cfMargin, cfPadding) could have template strings
If the variable is one of those types which could have a template string, run through a specific parser that will know how to handle the given type in the designTokenRegistry file. This way we can map specific parsers to specific variable types which may share or differ in their structure. Therefore as we add a new variable type (ie. border in a separate PR) we can refactor this code to generalize as we go along

Purpose

Stores the design token key instead of the hard coded value, and then is able to resolve the design token key and display the value instead.

Screenshot 2023-12-14 at 12 34 31 PM

TODOs

  • The other design tokens such as direction, size, border, background color
  • Pills to show the user that what they've selected is a design token

@ryunsong-contentful ryunsong-contentful force-pushed the ALT-171 branch 2 times, most recently from 3305738 to 20c3b6d Compare December 14, 2023 20:12
Copy link
Contributor

@primeinteger primeinteger left a comment

Choose a reason for hiding this comment

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

Nice work on this!

Copy link
Contributor

Choose a reason for hiding this comment

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

I've accidently generated this file before too, but since this repo only uses yarn, I think you can delete this

yarn.lock Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes to this file were possibly generated by npm. When this happens to me, I delete node_modules directory and regenerate the yarn.lock file using yarn

@@ -26,6 +28,8 @@ const toCSSMediaQuery = ({ query }: Breakpoint): string | undefined => {
return undefined;
};

const AvailableDesignTokenVariables = new Set(['cfPadding', 'cfMargin']);
Copy link
Contributor

Choose a reason for hiding this comment

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

The uppercase first letter is standard for component names and types/interfaces but uncommon for variable names.

Suggested change
const AvailableDesignTokenVariables = new Set(['cfPadding', 'cfMargin']);
const availableDesignTokenVariables = new Set(['cfPadding', 'cfMargin']);

Comment on lines 62 to 63
if (AvailableDesignTokenVariables.has(variableName)) {
if (variableName === 'cfMargin' || variableName === 'cfPadding')
Copy link
Contributor

Choose a reason for hiding this comment

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

The if statements could be combined

Suggested change
if (AvailableDesignTokenVariables.has(variableName)) {
if (variableName === 'cfMargin' || variableName === 'cfPadding')
if (AvailableDesignTokenVariables.has(variableName) && ['cfMargin', 'cfPadding'].includes(variableName)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I purposely did this so that when there's other attributes like border or whatever, I will add a separate clause for that so that I can direct that to the getDesignTokenRegistrationForBorder function. So this is the structure I want for the future even though today, in this PR you can shorten it like this. Hopefully what I wrote makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

      if (AvailableDesignTokenVariables.has(variableName)) {
        if (variableName === 'cfMargin' || variableName === 'cfPadding')
          return getDesignTokenRegistrationForSpacing(valuesByBreakpoint[breakpointId]);
        if(variableName === 'cfBorder')return 
          getDesignTokenRegistrationForBorder(valuesByBreakpoint[breakpointId]);
      }

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, makes sense 👍

If enough values are checked on the inner part, a switch might be preferable too

Comment on lines 32 to 35
const isTemplateStringFormat = (str: string) => {
if (!str || str.length < 3) return false;
return str.charAt(0) === '$' && str.charAt(1) === '{' && str.charAt(str.length - 1) === '}';
};
Copy link
Contributor

Choose a reason for hiding this comment

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

The regex you added to the resolveSpacingDesignToken method could be used here if you wanted

Suggested change
const isTemplateStringFormat = (str: string) => {
if (!str || str.length < 3) return false;
return str.charAt(0) === '$' && str.charAt(1) === '{' && str.charAt(str.length - 1) === '}';
};
const isTemplateStringFormat = (str: string) => {
const regex = /\$\{([^}]+)\}/g;
return regex.test(str);
};

@ryunsong-contentful ryunsong-contentful merged commit f92ddb6 into main Dec 14, 2023
2 checks passed
@ryunsong-contentful ryunsong-contentful deleted the ALT-171 branch December 14, 2023 23:27
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.

2 participants