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: postcss composes directive #2410

Merged
merged 9 commits into from
Sep 13, 2024
Merged

fix: postcss composes directive #2410

merged 9 commits into from
Sep 13, 2024

Conversation

eirikbacker
Copy link
Contributor

@eirikbacker eirikbacker commented Sep 13, 2024

@eirikbacker eirikbacker self-assigned this Sep 13, 2024
Copy link

changeset-bot bot commented Sep 13, 2024

⚠️ No Changeset found

Latest commit: ae8cfce

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Sep 13, 2024

Preview deployments for this pull request:

📖 Storybook 13. Sep 2024 - 13:30 (Norwegian time)

See all deployments at https://dev.designsystemet.no

Copy link
Contributor

github-actions bot commented Sep 13, 2024

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 63.75% 4590 / 7200
🔵 Statements 63.75% 4590 / 7200
🔵 Functions 86.11% 155 / 180
🔵 Branches 75.8% 586 / 773
File CoverageNo changed files found.
Generated in workflow #284

Copy link
Contributor

github-actions bot commented Sep 13, 2024

Preview deployments

Theme 13. Sep 2024 - 13:30 (Norwegian time)

@eirikbacker eirikbacker marked this pull request as ready for review September 13, 2024 09:35
@eirikbacker eirikbacker requested a review from unekinn September 13, 2024 09:38
@mimarz
Copy link
Collaborator

mimarz commented Sep 13, 2024

How do we want to handle multiple applies?

  1. @apply ds-focus, ds-paragraph--md;
  2. @apply ds-focus ds-paragraph--md;
  3. @apply ds-focus 
    @apply ds-paragraph--md;

@mimarz
Copy link
Collaborator

mimarz commented Sep 13, 2024

Do we have to fix the build for individual files? When the rule is not found it just keeps the @apply ds-focus.

@eirikbacker
Copy link
Contributor Author

Do we have to fix the build for individual files? When the rule is not found it just keeps the @apply ds-focus.

The individual files are not processed at all by postcss as you can see, so I'm a bit unsure why we add them to dist?

@eirikbacker
Copy link
Contributor Author

How do we want to handle multiple applies?

  1. @apply ds-focus, ds-paragraph--md;
  2. @apply ds-focus ds-paragraph--md;
  3. @apply ds-focus 
    @apply ds-paragraph--md;

I suggest the #3 approach - no problem making the others work, but it will require more code in your little plugin, and one apply class per line seems a bit cleaner too? ☺️

@eirikbacker eirikbacker requested a review from mimarz September 13, 2024 11:25
@mimarz
Copy link
Collaborator

mimarz commented Sep 13, 2024

After a short meeting between me and @eirikbacker we decided to tweak the @apply to a @composes with path to css file like css modules. @composes ds-focus from './utilities.css';

This will make it more clear and also work for single component css files having it applied correctly.

Copy link
Collaborator

@mimarz mimarz left a comment

Choose a reason for hiding this comment

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

Make sure to update the description and pr title with new directive name :)

@unekinn
Copy link
Contributor

unekinn commented Sep 13, 2024

Bra jobba! 👏

@eirikbacker eirikbacker changed the title fix: postcss apply directive fix: postcss composes directive Sep 13, 2024
@eirikbacker eirikbacker merged commit ab65142 into next Sep 13, 2024
8 checks passed
@eirikbacker eirikbacker deleted the fix/postcss-apply branch September 13, 2024 13:10
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.

Investigate CSS composition
3 participants