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

Packages: Adding RTL CSS support to the packages CSS #8187

Merged
merged 2 commits into from
Jul 25, 2018

Conversation

youknowriad
Copy link
Contributor

This PR adds RTL support to the packages CSS which allows us to drop the root package folders for all the packages that expose CSS files.

There's a new Webpack step copying the built CSS files from the packages folder into the build folder used by the WordPress registered styles.

Testing instructions

  • Check that the nux RTL CSS file is generated properly in build-style and copied to the global build folder.

@youknowriad youknowriad added [Type] Build Tooling Issues or PRs related to build tooling npm Packages Related to npm packages labels Jul 25, 2018
@youknowriad youknowriad self-assigned this Jul 25, 2018
@youknowriad youknowriad requested review from gziolo and a team July 25, 2018 09:15
@@ -98,8 +99,17 @@ function buildStyle( packagePath ) {
.then( ( result ) => callback( null, result ) );
};

const postCSSRTLSync = ( ltrCSS, callback ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Should we merge #8093 and refactor to use the asynchronous code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really care which one goes first but yeah, we should align

@@ -235,6 +236,13 @@ const config = {
'deprecated',
'dom-ready',
].map( camelCaseDash ) ),
new CopyWebpackPlugin(
gutenbergPackages.map( ( packageName ) => ( {
Copy link
Member

Choose a reason for hiding this comment

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

Nice and clean 👍

@gziolo
Copy link
Member

gziolo commented Jul 25, 2018

Almost there, in case of npm run build - output files aren't minified.

@gziolo gziolo added this to the 3.4 milestone Jul 25, 2018
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.

It works great, many thanks for fixing it.

🎉

@youknowriad youknowriad merged commit 66747a0 into master Jul 25, 2018
@youknowriad youknowriad deleted the add/rtl-css-support-to-packages branch July 25, 2018 11:38
@aduth
Copy link
Member

aduth commented Apr 7, 2019

Do we still need the WebpackRTLPlugin in the default configuration after this change? I'm trying to understand how the plugin is being used, because it appears to be dead code (per documentation, requires some complementing text extract plugin).

// Create RTL files with a -rtl suffix
new WebpackRTLPlugin( {
suffix: '-rtl',
minify: defaultConfig.mode === 'production' ? { safe: true } : false,
} ),

It's also the sole plugin we use which hasn't been updated for Webpack 4 and would break with the upcoming Webpack 5 release.

(node:54786) DeprecationWarning: Tapable.plugin is deprecated. Use new API on .hooks instead

@youknowriad
Copy link
Contributor Author

I think when we merged this PR, we were in a mixed where not all the packages were in the packages folder but I think now it not necessary anymore.

@aduth
Copy link
Member

aduth commented Apr 8, 2019

Okay, I'll plan to push up a pull request in the next day or so.

@aduth
Copy link
Member

aduth commented Apr 24, 2019

Just to make sure it's tracked, I created an issue at #15146. I expect to find some time to look at this on Friday, and have assigned myself accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants