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

Move transform styles function to blockEditor package. #15572

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented May 10, 2019

Description

This PR extracts a util function transformStyles available in WordPress/editor to an independent package.

This function does not depend on the editor module and this change enables us to remove the WordPress/editor dependency from the HTML block.

Removing the editor dependencies from blocks is a goal because in the widget screen editor will not be available.

How has this been tested?

I did some general smoke tests around the editor.
I verified the HTML block still works as before.
I executed wp.blockEditor.transformStyles( [ { css: 'h1, b { color: red; }' } ], '.wp' );in the browser console and I verified the output was: [".wp h1,↵.wp b {↵color: red;↵}"].
I executed wp.editor.transformStyles( [ { css: 'h1, b { color: red; }' } ], '.wp' ); to verify we are backcompatible.

@jorgefilipecosta jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Type] Code Quality Issues or PRs that relate to code quality [Feature] Widgets Screen The block-based screen that replaced widgets.php. labels May 10, 2019
@youknowriad
Copy link
Contributor

Is this something that make sense in the block editor module (provider or something else)? (I don't have the answer but I think it's something we need to at least think about).

@jorgefilipecosta
Copy link
Member Author

Is this something that make sense in the block editor module (provider or something else)? (I don't have the answer but I think it's something we need to at least think about).

I agree we need to discuss this change. This function needs lots of logic ast, traversing etc that is not directly related to the editor.
I imagine this util to transform CSS is generic enough that people may even want to use it outside of Gutenberg/WordPress that's on a reason for extracting it.
Besides this, I think an HTML block in the widget screen where the editor is not present may also need this functionality.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Thinking a bit more here. While everything else is generic the transformStyles function exported in packages/transform-styles/src/index.js is something opinionated and specific to the editor, I wonder if it should be exposed as is.

Maybe a first step for this PR is just to move these into the block editor module and expose the transformStyles function there?

@jorgefilipecosta jorgefilipecosta force-pushed the update/extract-transform-styles-function-to-independ-package branch from 8f149b5 to 0e1fa93 Compare May 13, 2019 14:47
@jorgefilipecosta jorgefilipecosta changed the title Extract transform styles function to independent package. Move transform styles function to blockEditor package. May 13, 2019
@jorgefilipecosta
Copy link
Member Author

Maybe a first step for this PR is just to move these into the block editor module and expose the transformStyles function there?

Hi @youknowriad the function was moved to the block editor module as suggested.

/**
* Internal dependencies
*/
export { default as transformStyles } from './transform-styles';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is looking good, my last question is whether we should publish this as is or make it experimental? I'm on the fence here as it seems to be a pretty stable API. Thoughts?

Also do we consider a change in the way we generate those styles as a breaking change? Say we automatically add a "width" to all blocks or something? (if it's the case I think it should be experimental or unstable)

cc @aduth @jorgefilipecosta @jasmussen

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like if you're on the fence, maybe keep it unstable for a release, then revisit?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with the idea of experimental for a release.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with the idea of experimental for a release.

Will you plan to follow-up on this, or should we create an issue to track it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @aduth, there is no need to create an issue, I will create a PR very soon, given that the experimental status should be removed in the next release.

@jorgefilipecosta jorgefilipecosta force-pushed the update/extract-transform-styles-function-to-independ-package branch from 0e1fa93 to d14d70f Compare May 28, 2019 11:03
@jorgefilipecosta
Copy link
Member Author

The PR was updated and block editor transformStyles was marked as experimental. Provided there is no other feedback, the PR should be ready.

@jorgefilipecosta jorgefilipecosta merged commit bb24bbe into master May 28, 2019
@jorgefilipecosta jorgefilipecosta deleted the update/extract-transform-styles-function-to-independ-package branch May 28, 2019 12:51
@youknowriad youknowriad added this to the Gutenberg 5.9 milestone Jun 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants