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

Fix CSS vendor-prefixed property serialization #7704

Merged
merged 1 commit into from Jul 5, 2018

Conversation

noisysocks
Copy link
Member

Description

I recently merged #7676 which attempted to improve how we handle serialising CSS properties. This PR made wp.element.renderToString() correctly serialise custom attribute properties, but was naive in its handling of elements that contain vendor-prefixed properties.

My confusion was that I didn't realise that, in React, you're supposed to write <div style={ { WebkitTransform: 'none' } }> and not <div style={ { '-webkit-transform': 'none' } }>.

The complete rules, inspired by these relevant React unit tests are:

  • Convert regular property names to kebab-case, e.g. backgroundColorbackground-color
  • Leave custom attributes alone, e.g. --myBackgroundColor--myBackgroundColor
  • Convert vendor-prefixed property names to -kebab-case, e.g. MozTransform-moz-transform

How has this been tested?

Unit tests have been updated.

You could also create a block that has returns a <div style={ { WebkitTransform: 'none' } }> in its save() function. Gutenberg should correctly save <div style="-webkit-transform:none"> to the post.

Corrects how wp.element.renderToString() serializes a vendor-prefixed
property such as MozTransform. These properties should be turned into
kebab-case and prefixed with a '-', e.g. '-moz-transform'.
@noisysocks noisysocks added [Type] Bug An existing feature does not function as intended [Package] Element /packages/element labels Jul 4, 2018
@noisysocks noisysocks requested review from gziolo and aduth July 4, 2018 05:54
@noisysocks
Copy link
Member Author

My confusion was that I didn't realise that, in React, you're supposed to write <div style={ { WebkitTransform: 'none' } }> and not <div style={ { '-webkit-transform': 'none' } }>.

Having said that, I'm not able to get Webkit properties to do anything when I try it in a CodePen

🤷‍♂️

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Code and tests make sense; weird that the pens don't work (I tried for WebKit and Moz prefixes in both browsers and got nothing as well).

But if the unit tests work and it's working in the editor that's good enough for me.

@noisysocks noisysocks merged commit 7c44995 into master Jul 5, 2018
@noisysocks noisysocks deleted the fix/css-vendor-prefixed-properties branch July 5, 2018 04:47
@ZebulanStanphill
Copy link
Member

Should this be added to the 3.2 milestone?

@ntwb ntwb added this to the 3.2 milestone Jul 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Element /packages/element [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants