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

Console redesign ground work - css refactor #6692

Merged
merged 10 commits into from
Nov 20, 2023

Conversation

ryaplots
Copy link
Contributor

@ryaplots ryaplots commented Nov 8, 2023

Summary

This PR updates the colors and introduces new utility classes.

References: https://github.com/TheThingsIndustries/product-management/issues/29

Changes

  • Add new utility classes
  • Add new colors
  • New icons
  • Use system font

Notes for Reviewers

To-do:

  • Test all components with styling changes and make sure they display correctly
  • Check the mixin.styl page to see if we still want to use all of them or we want to drop some
  • Fix compiling bug - after running tools/bin/mage js:serve and loading the page, takes a really long time to complete GET http://localhost:8080/console/ sometimes does not complete at all with error Command failed with signal "SIGABRT"

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • The steps/process to test this feature are clearly explained including testing for regressions.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@ryaplots ryaplots added the ui/web This is related to a web interface label Nov 8, 2023
@ryaplots ryaplots self-assigned this Nov 8, 2023
Copy link
Contributor

@kschiffer kschiffer left a comment

Choose a reason for hiding this comment

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

Very nice work. It's especially nice to see how much we can shave off the styling modules.

So indeed there is a problem now with the build time. This is very likely because of all the complex logic required to build the utility classes. I believe we have to look into caching these and only rebuild them when needed. The best way to do this, I believe, is to outsource all utility classes into a separate CSS module. Then it will only be rebuild when changed (which shouldn't happen often), all other modules can be bundled as usual. We just need to include both modules in the HTML then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, 8MB, that's massive. Is there a way to shrink this by only using some weights or so?

Copy link
Contributor Author

@ryaplots ryaplots Nov 14, 2023

Choose a reason for hiding this comment

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

The thing with the new Material Symbols is that they are a variable font, which means you can set whatever weights and styles you wish using FILL, GRAD, opsz and wght:

.material-symbols-outlined {
  font-variation-settings:
    'FILL' 1,
    'wght' 400,
    'GRAD' 0,
    'opsz' 0
}

https://fonts.google.com/knowledge/glossary/file_size

@kschiffer
Copy link
Contributor

kschiffer commented Nov 10, 2023

Also, I see that the resulting CSS is now waaaaaaay too big, with 1.7 million lines 😅, which also explains the build time issue. I believe we have some overzealous loop in the class generation there somewhere. Please have another look to see what it might be. It can happen quite quickly when generating classes per breakpoint, per size and per type all at once.

Also note that the TTUI CSS only has 825kb, as opposed to 32MB of this Console bundle.

EDIT: Looking through the bundle, I can see that there is a lot of duplicate definitions in there. Search for xl\:c-grey-700 for example and see that it gets redeclared over and over.

@ryaplots ryaplots marked this pull request as ready for review November 14, 2023 10:32
@ryaplots ryaplots requested a review from kschiffer November 14, 2023 10:32
Copy link
Contributor

@kschiffer kschiffer left a comment

Choose a reason for hiding this comment

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

Great!

@ryaplots ryaplots merged commit 7c5c50c into feature/console-redesign Nov 20, 2023
12 checks passed
@ryaplots ryaplots deleted the feature/css-groundwork branch November 20, 2023 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui/web This is related to a web interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants