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

fix: global css file reduce 5000 lines #7980

Closed

Conversation

alisonailea
Copy link
Contributor

Related Issue: #7325

Summary

calcite-design-tokens currently combines all modes and components into three files (headless, light, dark) which are then all included in the global.scss making the final global calcite.css file bloated.
There are some minor size improvements that can be made by utilizing design token references and removing duplications but the major issue is all of the component specific design tokens which do not need to be included in the global css file.

To remove this bloat the solution is...

  1. update the token-transformer to output better SCSS files which utilize @use and @forward to link related styles
  2. remove component tokens from global output
  3. stop relying on class based mode setting and get/set the mode via a local storage key which may be accessed and set as an attribute per component
  4. pull component tokens directly into the individual component styles

@alisonailea alisonailea force-pushed the astump/7325-global-css-file-reduce-5000-lines branch from 1c0bef0 to 2772014 Compare October 12, 2023 17:02
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Oct 12, 2023
@@ -10,6 +10,8 @@
"component/fab",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole file is outdated and set to be deleted.

@@ -3,297 +3,28 @@
"id": "59a65f6d4a3657738c9d46d61315ae0b6fc6e9fd",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole file is outdated and set to be deleted.

references: /(@\w+ "[\w\d.\/]+";)/g,
media: /(@media\s\([\w-]+:\s\w+\)\s*\{(\s*@include [\w.-]+\(\);)+\s*})/g,
root: /(:\w+(\(\[[\w-]+="\w+"\]\)){0,1} *\{ *@include [\w.-]+\(\); *\})/g,
classes: /(\.[\w\d-]+ \{(\s*@include [\w.-]+\(\); *)+\n*\})/g,

Check failure

Code scanning / CodeQL

Inefficient regular expression

This part of the regular expression may cause exponential backtracking on strings starting with '.- {{@include -();' and containing many repetitions of ' @include -();'.
@alisonailea
Copy link
Contributor Author

Some notes on the changes to tokens. You will notice a lot of the file changes are simply converting token references from the "$[token.reference.path]" to "{[token.reference.path]}". The use of the "$" to delineate token references was set in Token Studio by designers because it made their workflow easier within Figma. However, designers are no longer using Token Studio and the default file parser looks for "{}" token references. So, by converting to "{}" we remove the need for a custom parser, improving maintenance and performance.

componentWillLoad(): void {
// TODO: check if this attribute is manually set by the user before getting the theme set in local storage.
// TODO: make this a part of the generic component setup.
this.calciteMode = getMode();
Copy link
Member

Choose a reason for hiding this comment

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

This would likely need to happen on connectedCallback() so a component updates its mode if its moved position in the DOM.

@driskull
Copy link
Member

stop relying on class based mode setting and get/set the mode via a local storage key which may be accessed and set as an attribute per component

@alisonailea can you elaborate on why we would need to do this one?

I thought we would just be moving the component specific variables to the component files which would get placed into the CSS of the component instead of the global.css file therefore reducing the global.css file for users who are not using every component.

I have concerns with switching from using a CSS class approach to using an attribute. We had originally had a theme attribute but moved away from that because using CSS is a better practice than using an attribute for purely stylistic options. Using an attribute requires implementing our own cascading logic which is something that CSS just already does.

@alisonailea
Copy link
Contributor Author

alisonailea commented Oct 19, 2023

@driskull

@alisonailea can you elaborate on why we would need to do this one?

We will not be fully taking the approach listed in this draft as we don't currently want to rely on localstore. @jcfranco and I are working on a mutationObserver pattern solution similar to localization.

I thought we would just be moving the component specific variables to the component files

We still need a way to scope applying the mode specific tokens to the component in the CSS.

':host([calcite-mode="light"]): { ...design tokens as CSS custom props go here } '

I have concerns with switching from using a CSS class approach to using an attribute.

Without Container Style Queries (currently only available under a feature flag in chrome), using an attribute to specify the CSS is the only solution unless you want everything in global CSS.

@driskull
Copy link
Member

We still need a way to scope applying the mode specific tokens to the component in the CSS.

Can you share some example mode specific component tokens? I'd like to take a look at them. Maybe we could just keep these in global if there aren't too many.

@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Oct 27, 2023
@jcfranco
Copy link
Member

jcfranco commented Dec 5, 2023

This was addressed by #8215. Closing!

@jcfranco jcfranco closed this Dec 5, 2023
@jcfranco jcfranco deleted the astump/7325-global-css-file-reduce-5000-lines branch December 5, 2023 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. Stale Issues or pull requests that have not had recent activity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants