Skip to content

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

Merged
youknowriad merged 2 commits intomasterfrom
add/rtl-css-support-to-packages
Jul 25, 2018
Merged

Packages: Adding RTL CSS support to the packages CSS#8187
youknowriad merged 2 commits intomasterfrom
add/rtl-css-support-to-packages

Conversation

@youknowriad
Copy link
Copy Markdown
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 a team and gziolo July 25, 2018 09:15
.then( ( result ) => callback( null, result ) );
};

const postCSSRTLSync = ( ltrCSS, callback ) => {
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.

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

Copy link
Copy Markdown
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

'dom-ready',
].map( camelCaseDash ) ),
new CopyWebpackPlugin(
gutenbergPackages.map( ( packageName ) => ( {
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.

Nice and clean 👍

@gziolo
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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