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(examples): add tailwind preset example #1344

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jessicalynch
Copy link
Contributor

Adds example for building a Tailwind preset from tokens.

image

Original PR: #1036

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jessicalynch jessicalynch requested a review from a team as a code owner September 19, 2024 04:38
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1344.d16eby4ekpss5y.amplifyapp.com

Copy link
Collaborator

@jorenbroekema jorenbroekema 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 in general, comments are pretty easy ones to address I think.

Thanks for taking the effort to rebase your earlier PR to SD v4 :) much appreciated.

});

StyleDictionary.registerTransformGroup({
name: 'color/tailwind',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
name: 'color/tailwind',
name: 'tailwind',

I think our convention for transformGroups is that they are generally called after certain platform output types, and if we call it tailwind we can easily add other transforms that are necessary for tailwind output if needed in the future (e.g. for dimensions or whatever other types of tokens), without that being confusing.

tailwindPreset: {
buildPath: 'build/tailwind/',
transformGroup: 'color/tailwind',
usesDtcg: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm I think this can be left out, as Style Dictionary's auto-detection for whether a tokenset is DTCG or not is pretty decent and this way the example will also be valid for folks who just want to copy this example and paste in their own tokens which may not be DTCG formatted.

I kind of regret making this a user configurable platform option, in hindsight, but this may all change in a future v5 where hopefully we can just use DTCG at all times and just pre-convert people's tokens to DTCG if needed (see #1352)

Comment on lines +13 to +24
.join(',\n ');

return `import plugin from 'tailwindcss/plugin';

export default plugin(function ({ addBase }) {
addBase({
':root': {
${vars},
},
});
});\n`;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, this is a bit of a nitpick so feel free to ignore it, in fact, none of our examples or formats are doing this, but it occurred to me that it may be better to use tab characters over spaces:

Suggested change
.join(',\n ');
return `import plugin from 'tailwindcss/plugin';
export default plugin(function ({ addBase }) {
addBase({
':root': {
${vars},
},
});
});\n`;
};
.join(',\n\t\t\t');
return `import plugin from 'tailwindcss/plugin';
export default plugin(function ({ addBase }) {
\taddBase({
\t\t':root': {
\t\t\t${vars},
\t\t},
\t});
});\n`;
};

This would be more inclusive to user preferences (tab width, a11y reasons).

I'm just assuming this would be \t (tab character), but definitely test it for yourself to see if that works 😅 I haven't tested this.

If this ends up working well I'll raise an issue to improve this in all of our examples & templates, you'd have set the first proper example of it :)

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'll definitely try that out. I wasn't aware of the accessibility issues- thanks for sharing.

@@ -0,0 +1,10 @@
export const rgbChannels = (token, options) => {
const regex = /rgb\((\d+),\s*(\d+),\s*(\d+)\)/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should take into account that values may be of format rgba(r, g, b, a) (try it e.g. #FF00008C, the "color/rgb" transform outputs rgba(255, 0, 0, 0.55))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooo, good catch, I'll revise.

"license": "Apache-2.0",
"devDependencies": {
"style-dictionary": "^4.0.0",
"tailwindcss": "~3.4.12",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you expect breaking changes for this example in minor bumps for tailwind? Otherwise I'd go with ^3.4.12

"style-dictionary": "^4.0.0",
"tailwindcss": "~3.4.12",
"mocha": "^10.2.0",
"chai": "^5.0.0-alpha.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Stable v5 was released recently 🎉 so we can use ^5.1.1

I definitely appreciate you putting some tests in the tailwind example, that's neat!

@@ -0,0 +1,5 @@
/** @type {import('tailwindcss').Config} */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tailwind supports ESM which is the more modern format, so let's use that everywhere instead of CJS

The [Tailwind preset](https://tailwindcss.com/docs/presets#creating-a-preset) is imported from the build directory in `tailwind.config.js`.

```js
/** @type {import('tailwindcss').Config} */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as other comment, let's try using ESM over CJS

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