Skip to content

Let storybook be aware of postcss for .module.css files - #3518

Merged
maraisr merged 2 commits into
mainfrom
maraisr/postcss
Jul 17, 2023
Merged

Let storybook be aware of postcss for .module.css files#3518
maraisr merged 2 commits into
mainfrom
maraisr/postcss

Conversation

@maraisr

@maraisrmaraisr commented Jul 13, 2023

Copy link
Copy Markdown
Contributor

Storybook's addont-styling will autowire webpack for us to leverage css modules within our stories, and component files. Pathing the way to start using those as opposed to styled-components. We also prefer this addon now over storybook-css-modules.

Huge props to @siddharthkp for starting this work over in #3405

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Changes are SSR compatible
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@maraisrmaraisr added the skip changeset This change does not need a changelog label Jul 13, 2023
@maraisr
maraisr requested review from a team, langermank and siddharthkpJuly 13, 2023 23:25
@maraisrmaraisr self-assigned this Jul 13, 2023
@changeset-bot

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: af5cee2

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

@github-actions

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
dist/browser.esm.js101.91 KB (0%)
dist/browser.umd.js102.44 KB (0%)

@langermanklangermank left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving but would love for @siddharthkp to also approve before merging :)

@broccolinisoupbroccolinisoup left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is very exciting! Thanks for this PR. It looks good to me but I am only watching the changes for now as I don't have much context and experience on this area as much as Sid or Katie has :)

Comment threadpostcss.config.js
Comment on lines +8 to +9
'focus-visible-pseudo-class': false,
'logical-properties-and-values': false,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Curious to hear why they are false 👀

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Took inspiration from https://github.com/primer/view_components/blob/main/postcss.config.js possibly @langermank can provide more insights

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I see @simurai/@jonrohan worked on this file and found some comments here: https://github.com/primer/view_components/pull/1638/files#r1032151244

Interesting stuff.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In some of our cases, the output was putting more CSS than we needed for the browsers we supported. I think the focus-visible-pseudo-class was also adding a .focus class which was meant to be paired with JS. So it was on, until disabled.

@siddharthkpsiddharthkp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! Ship it

@maraisr
maraisr added this pull request to the merge queueJul 17, 2023
Merged via the queue into main with commit 2df4d4eJul 17, 2023
@maraisr
maraisr deleted the maraisr/postcss branch July 17, 2023 09:37
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip changesetThis change does not need a changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@maraisr@jonrohan@broccolinisoup@siddharthkp@langermank