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

Bundle PostCSS Global Data Plugin with default configuration #389

Merged
merged 12 commits into from
Apr 29, 2024

Conversation

fabiankaegy
Copy link
Member

@fabiankaegy fabiankaegy commented Apr 22, 2024

Related Issue/RFC: #340

Description of the Change

This change makes it possible to share one global configuration for things like custom breakpoints, selectors, mixins etc between all CSS entrypoints of a project.

Especially with the introduction of block specific stylesheets one common issue is that the global elements such as @custom-media definitions are not shared across all these individual entrypoints. This leads to unintended breakages when an engineer uses these variables thinking they will work just as expected from the main CSS file. But in reality this causes the styles to be broken because they cannot read the mixin that is only defined in the other entrpoint.

With this change both the custom media etc definitions, and any mixins get a new location in the theme located at ./assets/css/globals/ & ./assets/css/mixins/. All the .css files nested in that folder or any nested folders will now automatically get loaded and shared for all entrypoints.

These folders are configurable at the project level via either the paths.config.js file or the paths key in the 10up-toolkit object of package.json files. This flexibility to customize the paths also makes it possible to reference the folders of the theme in a plugin for example if you are developing a feature plugin that contains CSS and need to reference the themes media queries for example.

Note

This will require a change in the wp-scaffold once this has been released. I've created a draft PR for it 10up/wp-scaffold#228 However, this is not a breaking change :)

Alternate Designs

Initially, I wanted the globals to live outside of the theme folder at the monorepo root level to make it more clear that these elements are shared across all instances. But I think this here with the ability to customize it is the better experience.

Possible Drawbacks

This is a slight change in the development process that will need to be explained to the team. But it should be very nondisruptive.

Verification Process

The bundled sample theme project has been updated to use this new approach. Running the build in the theme directory will result in the mixins & globals getting applied both to the dist/css/frontend.css and the dist/blocks/example-block/style.css files.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.
  • I have added a changeset to my PR. See CONTRIBUTING document for instructions

Copy link

changeset-bot bot commented Apr 22, 2024

🦋 Changeset detected

Latest commit: 054ae44

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
10up-toolkit Minor

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

@fabiankaegy fabiankaegy linked an issue Apr 22, 2024 that may be closed by this pull request
@@ -0,0 +1,6 @@
---
"10up-toolkit": minor
"tenup-theme": minor
Copy link
Member

Choose a reason for hiding this comment

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

No need to patch the tenup-theme, you can remove this line from this file.

packages/toolkit/config/postcss.config.js Show resolved Hide resolved
packages/toolkit/config/postcss.config.js Show resolved Hide resolved
@fabiankaegy fabiankaegy added this to the [email protected] milestone Apr 22, 2024
Copy link
Member

@nicholasio nicholasio left a comment

Choose a reason for hiding this comment

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

@fabiankaegy I added a basic test but ran into a weird issue. Would love to figure it out before releasing a beta version of this.

background: #f5f5f5;
padding: 20px;

@mixin margin-trim;
Copy link
Member

Choose a reason for hiding this comment

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

@fabiankaegy I set up this test and for some reason, this line breaks building this file! It doesn't even create the css file.

Did I miss something? I'm wondering if this mixins is undefined and therefore breaking everyting. On the other hand I'd like to understand why this test doesn't even generate the css file with this line and it also doesn't give any error.

To run tests locally just do npm run test -w=packages/toolkit -- --watch you can run the tests then inspect the dist folder for each test to see what toolkit is outputting

Copy link
Member Author

Choose a reason for hiding this comment

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

@nicholasio it appears that the issue was due to the paths not being set correctly for the __fixtures__ path. See 52e5b67

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense! Thanks!

@nicholasio nicholasio merged commit 1693913 into develop Apr 29, 2024
7 checks passed
@nicholasio nicholasio deleted the feature/postcss-global-data branch April 29, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Bundle PostCSS Global Data Plugin with default configuration
2 participants