-
Notifications
You must be signed in to change notification settings - Fork 36
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
refactor(cli): declarative build configuration #2633
base: next
Are you sure you want to change the base?
Conversation
|
Preview deployments for this pull request: Storybook - |
Coverage Report
File CoverageNo changed files found. |
b4a096e
to
d4fb0de
Compare
46a902c
to
b79da12
Compare
5f2c88e
to
49e0e58
Compare
} | ||
const result: ProcessedThemeObject = { ...theme, [processed]: true }; | ||
if (result.group) { | ||
result.group = camelCase(result.group); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we use the same casing for name
and group
on our themes so its more predictable to use in code. Ideally lowerCase, but maybe noCase
since we might have group names with spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does noCase
actually do? I like camelCase
since it is quite predictable:
import camelCase from 'camelcase';
camelCase('foo-bar');
//=> 'fooBar'
camelCase('foo_bar');
//=> 'fooBar'
camelCase('Foo-Bar');
//=> 'fooBar'
camelCase('--foo.bar');
//=> 'fooBar'
camelCase('foo bar');
//=> 'fooBar'
camelCase('Foo bar');
//=> 'fooBar'
camelCase(['__foo__', '--bar']);
//=> 'fooBar'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noCase
is just kebab-case
with some additional parsing for replacing spaces. Maybe we should rename it then?
Its something thats been there since before I started and we still had some design-tokens we needed it for.
I think whats most important is we use the same case conversion for our fields for further use. Since we've already started using kebab-case for folders in design-tokens and names I feel its maybe the best fit?
packages/cli/src/tokens/build/utils/getMultidimensionalThemes.ts
Outdated
Show resolved
Hide resolved
packages/cli/src/tokens/build.ts
Outdated
const semanticThemes = R.filter((val) => val.mode === 'light' && val.typography === 'primary', themes); | ||
// We only use the 'default' theme for the 'size' group | ||
const relevant$themes = $themes.filter((theme) => | ||
R.not(theme.group === 'size' && theme.name.toLowerCase() !== 'default'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix for potential pitfall filtering themeobjects from $themes
R.not(theme.group === 'size' && theme.name.toLowerCase() !== 'default'), | |
R.not(theme.group.toLowerCase() === 'size' && theme.name.toLowerCase() !== 'default'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unneccessary because the names are normalised in processTheme
. Actually I should have removed the other toLowerCase()
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But these names are not normalised in this filter since its dones before processTheme
. Its done straight on the raw ThemeObjects from $themes.json
Looking at it now this filter does not work for our own design-tokens
as group is uppercase Size
.
Removing it does not alter the build output either so suggest just removing as it seems redundant now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true! I guess I didn't complete my thought process, I was planning to move processTheme
earlier in the "pipeline".
It works right now because the permutation with size: "default"
comes before size: "compact"
, due to the order they are defined in $themes.json. Since none of the build configurations depend on the size
dimension, the first one is always used.
This automates the config filtering which so far has been done manually, e.g. this ```ts const semanticThemes = R.filter((val) => val.mode === 'light' && val.typography === 'primary', themes) ``` is now this ``` const semanticThemes = getThemesFor('semantic'); ``` The main reason for doing this is that it requires no changes to the filters when adding new modes, while the code as it is before this commit would require every `R.filter(...)` statement to be updated to ignore irrelevant modes -- or suffer exponential increase in build time. The resulting css files from the build command remain unchanged.
49e0e58
to
2792ebd
Compare
This started as a refactor allowing this
to become this
The main reason for doing this was that it requires no changes to the filters when adding new modes, while the code as it is before this commit would require every
R.filter(...)
statement to be updated to ignore irrelevant modes -- or suffer exponential increase in build time.Declarative config
However, after some issues with moving the definition of
getThemesFor
out offunction buildTokens(...)
I started refactoring the build script more generally. This ended up with a declarative config defined inbuild.ts
, which looks like thisNote the
BuildConfig
type definition, which isThe resulting css files from the build command remain unchanged.