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.

Sign up for GitHub

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

Jump to bottom

Fix previews for blocks using InnerBlocks #15561

Merged
merged 2 commits into from
May 16, 2019
Merged

Fix previews for blocks using InnerBlocks #15561

merged 2 commits into from
May 16, 2019

Conversation

youknowriad
Copy link
Contributor

closes #9897 Alternative to #14767

This PR uses an inner block editor component to render the preview.

Testing instructions

  • Add a style variations for the columns block, group block
styles: [
		{ name: 'default', label: _x( 'Default', 'block style' ), isDefault: true },
		{ name: 'outline', label: __( 'Outline' ) },
		{ name: 'squared', label: _x( 'Squared', 'block style' ) },
	],
  • Check that you can switch style variations.

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Feature] Theme Style Variations Related to style variations provided by block themes labels May 10, 2019
@youknowriad youknowriad self-assigned this May 10, 2019
@youknowriad youknowriad mentioned this pull request May 10, 2019
Closed
gziolo
gziolo reviewed May 10, 2019
View reviewed changes
packages/block-editor/src/components/block-preview/index.js
attributes={ block.attributes }
setAttributes={ noop }
/>
<BlockEditorProvider
Copy link
Member

Choose a reason for hiding this comment

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

That's neat ❤️

gziolo
gziolo reviewed May 15, 2019
View reviewed changes
Copy link
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.

Code looks good but the visual part has some flaws:

Screen Shot 2019-05-15 at 16 57 50

youknowriad
youknowriad commented May 15, 2019
View reviewed changes
packages/block-editor/src/components/block-preview/style.scss
@@ -29,6 +29,17 @@
}
}

.block-editor-block-preview__content {
// Resetting the block editor paddings and margins
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's not the ideal fix as I think by default the block editor shouldn't have paddings and margins but It's not easy to reset without big implications. I'm fixing it for the preview right now.

Copy link
Member

Choose a reason for hiding this comment

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

@jasmussen or @kjellr - any suggestions on how to approach it nicer? I'm fine with the proposal myself but I know little about proper CSS :)

@youknowriad
Copy link
Contributor Author

@gziolo Good catch, It should be better now.

gziolo
gziolo approved these changes May 16, 2019
View reviewed changes
Copy link
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 looks much better, but generally speaking, this preview could get some improvements as it doesn't look optimal for more complex blocks:

Screen Shot 2019-05-16 at 11 05 32
Screen Shot 2019-05-16 at 11 05 40

packages/block-editor/src/components/block-preview/style.scss
@@ -29,6 +29,17 @@
}
}

.block-editor-block-preview__content {
// Resetting the block editor paddings and margins
Copy link
Member

Choose a reason for hiding this comment

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

@jasmussen or @kjellr - any suggestions on how to approach it nicer? I'm fine with the proposal myself but I know little about proper CSS :)

@gziolo gziolo added this to the 5.8 (Gutenberg) milestone May 16, 2019
@gziolo gziolo requested review from kjellr and jasmussen May 16, 2019 09:08
@youknowriad
Copy link
Contributor Author

@gziolo I agree that sometimes a preview in such a small area is not the best solution. That said I'm happy this PR fixes the style variation issues for nested blocks.

@gziolo
Copy link
Member

gziolo commented May 16, 2019

@gziolo I agree that sometimes a preview in such a small area is not the best solution. That said I'm happy this PR fixes the style variation issues for nested blocks.

Yep, we should definitely land it. I wanted to make sure it gets noted by designers as well so they could include it in their future plans :D

@youknowriad youknowriad merged commit f48a325 into master May 16, 2019
@youknowriad youknowriad deleted the fix/preview-nested-blocks branch May 16, 2019 10:03
@kjellr kjellr mentioned this pull request May 17, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo approved these changes

@ellatrix ellatrix Awaiting requested review from ellatrix

@noisysocks noisysocks Awaiting requested review from noisysocks

@talldan talldan Awaiting requested review from talldan

@getdave getdave Awaiting requested review from getdave

@swissspidy swissspidy Awaiting requested review from swissspidy

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@kjellr kjellr Awaiting requested review from kjellr

@jasmussen jasmussen Awaiting requested review from jasmussen

Assignees

@youknowriad youknowriad

Labels
[Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Feature] Theme Style Variations Related to style variations provided by block themes [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
5.8 (Gutenberg)
Development

Successfully merging this pull request may close these issues.

Styles variations don't work for blocks using InnerBlocks
3 participants
@youknowriad @gziolo @getdave