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

Scripts: Allow CSS modules in the build commands #29182

Merged
merged 4 commits into from
Apr 8, 2021
Merged

Scripts: Allow CSS modules in the build commands #29182

merged 4 commits into from
Apr 8, 2021

Conversation

cpiber
Copy link
Contributor

@cpiber cpiber commented Feb 20, 2021

Description

@wordpress/scripts modified config to allow css modules (and extract them like style.css)

Checklist:

  • My code is tested.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Feb 20, 2021
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @cpiber! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@gziolo gziolo added [Status] Needs More Info Follow-up required in order to be actionable. [Tool] WP Scripts /packages/scripts labels Feb 22, 2021
@gziolo
Copy link
Member

gziolo commented Feb 22, 2021

Hi @cpiber. Thank you for your contribution. Can you provide more details for the proposed change?

  • What issue is it trying to solve?
  • How it can be tested?
  • How it should be documented?

@cpiber
Copy link
Contributor Author

cpiber commented Feb 22, 2021

  • It allows for using css modules. The webpack plugin css-loader already supports that.
  • I'm not sure how you usually set webpack configurations. The changes I made will compile .*.module.(sa|sc|c)ss files with css modules enabled. They also extract style(.module)?.(sc|sa|c)ss files (so module files as well).
  • In the readme there is the "Using CSS" section, possibly mention the use of css-modules there as well.

@gziolo gziolo removed the [Status] Needs More Info Follow-up required in order to be actionable. label Feb 22, 2021
Base automatically changed from master to trunk March 1, 2021 15:45
@gziolo
Copy link
Member

gziolo commented Apr 1, 2021

In the readme there is the "Using CSS" section, possibly mention the use of css-modules there as well.

That would be a great addition with an example of the file and its content maybe? There is already a section with CSS:
https://github.com/WordPress/gutenberg/tree/trunk/packages/scripts#using-css

I guess once we have documentation. It only needs to be tested and we are good to go. Thank you for your contribution 🙇🏻

@gziolo gziolo added the [Type] New API New API to be used by plugin developers or package users. label Apr 1, 2021
@gziolo gziolo changed the title packages/scripts: config allow css modules Scripts: Allow CSS modules in the build commands Apr 1, 2021
@cpiber
Copy link
Contributor Author

cpiber commented Apr 1, 2021

To clarify, do you want me to edit the README?

@gziolo
Copy link
Member

gziolo commented Apr 1, 2021

To clarify, do you want me to edit the README?

Yes, that would be helpful for folks using wp-scripts.

A section New Feature in the changelog would also help with discovery:

https://github.com/WordPress/gutenberg/blob/trunk/packages/scripts/CHANGELOG.md

@gziolo
Copy link
Member

gziolo commented Apr 2, 2021

It looks like you merged your branch with the upstream master branch. It was renamed to trunk so that would explain why there are additional 400+ commits in your PR 😄

@cpiber
Copy link
Contributor Author

cpiber commented Apr 2, 2021

I rebased on trunk, yes

@gziolo
Copy link
Member

gziolo commented Apr 3, 2021

I'm not sure what went wrong but there should be only your changes listed in this PR. You can try following this documentation and see if it helps:
https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/git-workflow.md#keeping-your-fork-up-to-date

@cpiber
Copy link
Contributor Author

cpiber commented Apr 3, 2021

I used rebase instead of merge, maybe that was a problem? Should I go back to an earlier commit and try with merge again?

@gziolo
Copy link
Member

gziolo commented Apr 3, 2021

You can also cherry-pick your commits to a fresh branch if that helps. Whatever makes it work and I can merge your changes 😄

@cpiber
Copy link
Contributor Author

cpiber commented Apr 3, 2021

Alright, I went back to the old master and did a merge instead of a rebase, I hope it's okay now

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. This looks good to go 🎉

@gziolo gziolo merged commit a2e284d into WordPress:trunk Apr 8, 2021
@github-actions
Copy link

github-actions bot commented Apr 8, 2021

Congratulations on your first merged pull request, @cpiber! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@github-actions github-actions bot added this to the Gutenberg 10.5 milestone Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Tool] WP Scripts /packages/scripts [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants