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

Add default block style if missing #12519

Merged
merged 2 commits into from Jan 29, 2019

Conversation

Projects
None yet
4 participants
@swissspidy
Copy link
Member

swissspidy commented Dec 2, 2018

Description

This attempts to fix #11613 by adding a default block style if block styles were added to a block which doesn't have any by default.

How has this been tested?

Only manual testing so far. I could need some assistance if E2E tests or similar are needed.

Types of changes

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

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@swissspidy

This comment has been minimized.

Copy link
Member Author

swissspidy commented Dec 9, 2018

@youknowriad Would love to get your thoughts on this

onSwitch = noop,
onHoverClassName = noop,
} ) {
if ( ! styles || styles.length === 0 ) {
return null;
}

if ( ! type.styles && ! find( styles, 'isDefault' ) ) {

This comment has been minimized.

@youknowriad

youknowriad Dec 9, 2018

Contributor

Is this ! type.styles really necessary?

This comment has been minimized.

@swissspidy

swissspidy Dec 9, 2018

Author Member

This condition checks whether the block type originally contains styles, whereas styles contains all styles (original styles + ones added through registerBlockStyle).

The default style only needs to be added when the block type didn't already contain styles in the beginning.

So yeah, it's necessary :-)

This comment has been minimized.

@youknowriad

youknowriad Dec 9, 2018

Contributor

The default style only needs to be added when the block type didn't already contain styles in the beginning.

I'm not certain I understand why? Why can't we always add a default style anytime we're missing one?

This comment has been minimized.

@swissspidy

swissspidy Dec 9, 2018

Author Member

Right. Yeah I guess I did some wrong thinking there 🤔

@youknowriad
Copy link
Contributor

youknowriad left a comment

Sounds reasonable to me. I'm temptatively adding it to 4.8.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 9, 2018

some tests (unit or e2e) would be good :)

@youknowriad youknowriad added this to the 4.8 milestone Dec 9, 2018

@swissspidy

This comment has been minimized.

Copy link
Member Author

swissspidy commented Dec 9, 2018

Cool. I'll try to add some tests. Is https://wordpress.org/gutenberg/handbook/contributors/testing-overview/ still the best reference for that?

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 12, 2018

yes, the tests docs seems good. Let me know if you need help figuring it out.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 14, 2018

@swissspidy you can ping me if you need any help with tests :)

@youknowriad youknowriad modified the milestones: 4.8, 4.9 Dec 19, 2018

@youknowriad youknowriad merged commit d1c78bf into master Jan 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 29, 2019

Let's follow up with tests separately.

@youknowriad youknowriad deleted the add/dynamic-default-styles branch Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment