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 overwriting existing line height style when none has been set in CoBlocks #1526

Merged

Conversation

p-jackson
Copy link
Contributor

@p-jackson p-jackson commented Jun 9, 2020

Description

The latest version of Gutenberg includes typography settings which already exist in CoBlocks. This PR doesn't fully harmonise those settings, but it stops CoBlock from disabling the core settings.

Using line-height as an example: if the line height was unset in CoBlocks, then applyStyle() would set the inline style to null. This means that if core Gutenberg had set it's own line height then it would get nulled out. As shown in #1500 this caused the line height to be removed on the front end and could cause invalid block errors.

This PR only sets an inline typography style if one has been set in CoBlocks, otherwise it leaves it alone. This means the CoBlocks setting takes precedent over the core Gutenberg setting on the front end. Maybe it should be the other way round, but this at least fixes the invalid block errors.

It's possible to do this without the _.pickBy function but it's more verbose and I saw this function was already used elsewhere in the plugin. From a code size point of view.

Fixes #1500

Types of changes

Bug fix (non-breaking change which fixes an issue)

How has this been tested?

  • Enable latest Gutenberg and disable CoBlocks
  • Add a paragraph block to a post
  • Set a custom line height in the block settings
  • Enable CoBlocks
  • Open post, should have no block errors
  • Preview post, line height should be applied

And checked the other way round just to be sure

  • Enable CoBlocks and disable latest Gutenberg
  • Add a paragraph block to a post
  • Set a custom line height using the CoBlocks settings in the toolbar
  • Enable latest Gutenberg
  • Open post, should have no block errors
  • Add a different line height using core block settings
  • Preview post, line height from CoBlocks settings should be applied

Also tried the above using the other blocks that have custom typograph settings.

Checklist:

  • My code is tested
  • My code follows accessibility standards
  • My code has proper inline documentation

@p-jackson p-jackson changed the title Fix overwrite core line height Fix overwriting existing line height style when none has been set in CoBlocks Jun 9, 2020
@p-jackson p-jackson marked this pull request as ready for review June 9, 2020 05:16
@AnthonyLedesma AnthonyLedesma added the [Type] Bug Something that is not working as expected label Jun 9, 2020
Copy link
Member

@AnthonyLedesma AnthonyLedesma left a comment

Choose a reason for hiding this comment

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

Great work. This resolves the issue reported in #1500 and is good to merge.

@richtabor richtabor added this to the 2.0.0 milestone Jun 9, 2020
@richtabor richtabor merged commit c4c3b3c into godaddy-wordpress:master Jun 9, 2020
@AnthonyLedesma
Copy link
Member

Though I approve this PR the work needed for typography controls is not yet completed. A new properly scoped issue details the outstanding required fixes with typography controls.
#1528

@p-jackson p-jackson deleted the fix-overwrite-core-line-height branch June 9, 2020 22:04
@jrtashjian jrtashjian linked an issue Jun 25, 2020 that may be closed by this pull request
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug Something that is not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gutenberg's line-height setting doesn't work if CoBlocks is activated
3 participants