-
Notifications
You must be signed in to change notification settings - Fork 70
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
Compress atomic class names #1408
Conversation
🦋 Changeset detectedLatest commit: 62aab19 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
TODO
|
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
'_aaaacccc', | ||
], | ||
[ | ||
'should ensure the last atomic declaration of a single group with short class name wins', |
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 haven't followed up the previous PRs and I am still wrapping my head around these changes. Hence, apologies if this was mentioned somewhere else
I am assuming that:
_aaaabbbb
is the CSS class with no manipulationa
is the optimised class, which is stored in the map_aaaa_a
is made of atomic group + optimised class -> in this way we are able to handle overrides
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.
Yes, spot on!
@@ -0,0 +1,27 @@ | |||
{ | |||
"1wyb12am": "a", |
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.
Where/how does this map get generated?
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.
Good question! For now it's assumed that library consumers manually curate the map and pass it to the babel-plugin. We will later create an automatic method to generate the map by statically analysing the codebase, and the map should be periodically updated by a build.
@@ -201,4 +201,166 @@ describe('babel plugin', () => { | |||
|
|||
expect(actual).toInclude('c_MyDiv'); | |||
}); | |||
|
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.
Do we still have cases where compression is not safe, hence we should bail out?
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.
Compression should be safe everywhere now.
…s to support conditional CSS
builtCss.css.map((x) => getItemCss(x)).join(''), | ||
meta.state.opts | ||
); | ||
const { sheets, classNames } = transformCssItems(builtCss.css, meta); |
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.
The change in this file is to ensure <ClassNames>
handles conditional CSS rules.
i.e.
<ClassNames>
{({ css }) => (<div className={css({ ...props.isPrimary && { color: 'blue' }})}>hello, world!</div>)}
</ClassNames>
const compressedClassName = | ||
classNameCompressionMap && classNameCompressionMap[fullClassName.slice(1)]; | ||
|
||
if (compressedClassName) { |
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.
If compressed class name is available, we are replacing full class name with the compressed in the sheet.
So
.aaaabbbb { font-size: 10px }
becomes
.a { font-size: 10px }
It will reduce the size of stylesheet but will break if we don't replace every occurrence of _aaaabbbb
, which I should have done in this PR. 🤞
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Bump node version to v18 * Bump import jsx * Bump nvmrc node version from 18.12 to 18.14 * Add changeset for PR #1392 --------- Co-authored-by: Grant Wong <[email protected]>
* chore(deps): update parcel packages * Add dummy generic type so optimizer works with parcel v2.8.0+ Parcel v2.8.0 adds a BundleConfigType generic type to Optimizer (parcel-bundler/parcel#8370) as part of their `loadBundleConfig` method. We use a dummy type because we don't need this functionality currently. * Update snapshot tests for Parcel 2.8.3 --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Grant Wong <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* chore(deps): update dependency jest to v29 * Update snapshots and node env for some tests --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Jake Lane <[email protected]>
Co-authored-by: Grant Wong <[email protected]>
…s to support conditional CSS
Thank you Liam for updating the PR :) This looks good to me, but I'll wait for an approval from @JakeLane or @pancaspe87 too since I don't have a good understanding of the whole codebase |
This PR does