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

feat(*): add css scoping to each block #703

Merged
merged 4 commits into from
Jan 9, 2024
Merged

Conversation

fulopdaniel
Copy link
Contributor

Created a PostCSS Plugin that prepends every css rule in the generated style.css with an extra selector, essentially scoping the whole css file to the specific block.

.foo .bar { ... }

becomes

.text-block .foo .bar { ... }

Previously we relied on Tailwind's important scoping approach, but that does not scope css rules that were imported to the package (e.g. @frontify/guideline-blocks-settings/dist/style.css)

To test:

  1. run pnpm run deploy --dryRun inside any package
  2. check the style.css inside the /dist folder of the package
  3. all rules should be scoped

@fulopdaniel fulopdaniel marked this pull request as ready for review January 9, 2024 13:20
@fulopdaniel fulopdaniel requested a review from a team as a code owner January 9, 2024 13:20
@fulopdaniel fulopdaniel marked this pull request as draft January 9, 2024 13:20
@fulopdaniel
Copy link
Contributor Author

@hochreutenerl
Copy link
Contributor

Nice direction where this is going. @fulopdaniel
Would it even be possible to scope with the appId (read it from the manifest), so we can have a solution that also works for external blocks?

Copy link
Collaborator

@ragi96 ragi96 left a comment

Choose a reason for hiding this comment

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

And I noticed that the css build now is smaller and I don't get why thats the case as it should be bigger, are we missing something?

postcss/scope.js Outdated Show resolved Hide resolved
postcss/scope.js Outdated Show resolved Hide resolved
@ragi96 ragi96 self-requested a review January 9, 2024 14:28
Copy link
Collaborator

@ragi96 ragi96 left a comment

Choose a reason for hiding this comment

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

Sorry, fat finger approved it.

@fulopdaniel
Copy link
Contributor Author

fulopdaniel commented Jan 9, 2024

@hochreutenerl We could read the manifest.json, but that does not automatically solve the issue for external blocks. They will need to configure their postcss.config.js themselves either way, and then also add that class to their block.

@ragi96 I will look into the point with the css size. One potential reason why the bundle could be smaller is that I removed the tailwind important part, so that is no longer part of the bundle.

Update: The CSS is has the same amount of lines with and without the plugin. The tailwind 'important' approach duplicated some rules, that's why the previous bundle was bigger.

@fulopdaniel fulopdaniel requested a review from ragi96 January 9, 2024 14:53
@fulopdaniel fulopdaniel marked this pull request as ready for review January 9, 2024 15:45
Copy link
Collaborator

@ragi96 ragi96 left a comment

Choose a reason for hiding this comment

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

LGTM

@fulopdaniel fulopdaniel merged commit 75e07ba into main Jan 9, 2024
10 checks passed
@fulopdaniel fulopdaniel deleted the feat/postcss-scoping branch January 9, 2024 15:47
@hochreutenerl
Copy link
Contributor

@fulopdaniel I think we can also adjust our templates - not all of them use postcss though - and add a matching class to wrapping div.
It's fine for me to do it as a later stage.

@ragi96
Copy link
Collaborator

ragi96 commented Jan 9, 2024

@hochreutenerl then we probably should release the plugin properly so we don't have to copy paste the code

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.

3 participants