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

remove css imports from components #1322

Merged
merged 7 commits into from
Jun 6, 2023
Merged

remove css imports from components #1322

merged 7 commits into from
Jun 6, 2023

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented Jun 1, 2023

Background

Up until now, we were importing .css files inside our React components. Such imports are not valid javascript syntax, and we have been relying on bundlers to magically take care of it. In hindsight, this was a mistake, just like automatically setting theme on the document was a mistake (eventually fixed in iTwin/iTwinUI-react#825 and #1265).

Some of the many issues with using such non-standard imports:

Changes

Removed all css imports from every component and created a styles.css file that can be manually imported. (This change was initially part of #1302 but pulled out for ease of review.)

Testing

All tests/playgrounds been updated to explicitly import the css.

Docs

Updated readme/md pages and added changeset.

@mayank99 mayank99 self-assigned this Jun 1, 2023
@mayank99 mayank99 force-pushed the mayank/css-imports branch from e03d97b to 8ad4039 Compare June 1, 2023 15:02
@veekeys
Copy link
Member

veekeys commented Jun 1, 2023

So now we will always import all the css even if we use few components from iTwinUI ? :(

@mayank99
Copy link
Contributor Author

mayank99 commented Jun 1, 2023

So now we will always import all the css even if we use few components from iTwinUI ? :(

Yes. This is actually a requirement for CSS modules. We cannot scope every component separately, as the same class when used in two different css files would get different hashes.

If you're concerned about bundle size, it's only 30KB gzipped and easily cacheable.

@mayank99 mayank99 marked this pull request as ready for review June 1, 2023 15:50
@mayank99 mayank99 requested review from a team as code owners June 1, 2023 15:50
@mayank99 mayank99 requested review from gretanausedaite and LostABike and removed request for a team June 1, 2023 15:50
@mayank99 mayank99 mentioned this pull request Jun 1, 2023
2 tasks
@mayank99 mayank99 force-pushed the mayank/css-imports branch from 737371c to 52144fe Compare June 1, 2023 16:32
@mayank99 mayank99 added this to the React 3.0 milestone Jun 1, 2023
Copy link
Contributor

@LostABike LostABike left a comment

Choose a reason for hiding this comment

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

I think this is good now. I have checked the visuals and they look good/not broken anymore. Also checked code changes and Overview page 👍

Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

Lets do dev release with this and css modules and try to migrate real app (eg. AppUI with its test apps)

@mayank99
Copy link
Contributor Author

mayank99 commented Jun 6, 2023

Lets do dev release with this and css modules and try to migrate real app (eg. AppUI with its test apps)

Do you mean from a branch? Because we have changesets set up, so merging the PR will also allow doing a dev release.

@gretanausedaite
Copy link
Contributor

gretanausedaite commented Jun 6, 2023

Do you mean from a branch? Because we have changesets set up, so merging the PR will also allow doing a dev release.

No, with changesets. Just saying it's a good idea to test this in real app.

@mayank99 mayank99 added this pull request to the merge queue Jun 6, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 6, 2023
@mayank99 mayank99 added this pull request to the merge queue Jun 6, 2023
@mayank99 mayank99 removed this pull request from the merge queue due to a manual request Jun 6, 2023
@mayank99 mayank99 enabled auto-merge June 6, 2023 14:23
@mayank99 mayank99 added this pull request to the merge queue Jun 6, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 6, 2023
@mayank99 mayank99 merged commit f4cb394 into dev Jun 6, 2023
@mayank99 mayank99 deleted the mayank/css-imports branch June 6, 2023 15:07
@imodeljs-admin imodeljs-admin mentioned this pull request Oct 23, 2023
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.

4 participants