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

Tiled gallery: Fix infinite loop left/right block align #29017

Merged
merged 1 commit into from
Dec 3, 2018

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Dec 1, 2018

Fixes and infinite loop with the resize observer that causes the block to shrink indefinitely when left or right aligned.

Note: Some alignments do not appear to be implemented correctly. This PR does slightly change the alignment, in particular for the "full" alignment. It's unclear to me what the appropriate styling is for center, full and wide alignments, but these should be revisited in a follow-up.

Changes proposed in this Pull Request

  • Use adjacent sibling selectors to add margins between items and rows rather than relying on adding margins around items and subtracting them

Testing instructions

  • Insert the block.
  • Try left and right alignment. Does it work correctly?
  • Is the item and row spacing the same as before?
  • Is the vanishing block bug gone?

Screens

Before (from #29016 - what a fast, smooth infinite loop 😍 )

screen shot 2018-12-01 at 23 49 12

before

After

screen shot 2018-12-01 at 23 49 49

after

Fixes and infinite loop with the resize observer
@sirreal sirreal added [Type] Bug Jetpack [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Design [Goal] Gutenberg Working towards full integration with Gutenberg [Block] Tiled Gallery labels Dec 1, 2018
@matticbot
Copy link
Contributor

@sirreal sirreal self-assigned this Dec 1, 2018
@@ -4,5 +4,5 @@ $tiled-gallery-add-item-border-color: #555d66; // Gutenberg $dark-gray-500
$tiled-gallery-add-item-border-width: 1px; // Gutenberg $border-width
$tiled-gallery-caption-background-color: #000;
$tiled-gallery-content-width: 610px; // Gutenberg $content-width
$tiled-gallery-margin: 2px; // Fixed in JS, see `LayoutStyles` from `edit.jsx`
$tiled-gallery-gutter: 4px; // Fixed in JS, see `LayoutStyles` from `edit.jsx`
Copy link
Member

Choose a reason for hiding this comment

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

Let's loop back later to rename margin to gutter also elsewhere to keep things consistent:

@simison
Copy link
Member

simison commented Dec 3, 2018

Core Gallery spacing was improved in WordPress/gutenberg#10027 (and WordPress/gutenberg#10221, also maybe https://github.com/WordPress/gutenberg/commits/master/packages/block-library/src/gallery/style.scss#diff-cf158936847e50c10bf9797499a7247dR127) — should we follow the suit while at it?

Bringing those changes over might fix the issue as well.

@simison
Copy link
Member

simison commented Dec 3, 2018

This does fix the bug 💪

Note: Some alignments do not appear to be implemented correctly. This PR does slightly change the alignment, in particular for the "full" alignment. It's unclear to me what the appropriate styling is for center, full and wide alignments, but these should be revisited in a follow-up.

Yup, these seem to need some work indeed. For the record:

This PR changes slightly how "Full width" behaves on TwentyNineteen theme:

Probably hard to see from screenshots but in master border is cut little bit while in this PR it's fully visible. The left border is hidden in both and that's; I'm not sure if that's how it should actually behave.

image

image

In core-gallery borders stay hidden on both sides:

screenshot 2018-12-03 at 11 42 35

screenshot 2018-12-03 at 11 42 31

On full width, this PR also pushes the content just a little bit under the menu, as demonstrated by this insert shadow in <figure> element:

master
screenshot 2018-12-03 at 11 37 12

fix/g7g-tg-layout-renderers
screenshot 2018-12-03 at 11 38 12

The same seems to happen in core-gallery block:

image

Tested by applying box-shadow: inset 0 0 0 5px red; to figure and setting opacity:0 in the img and caption.

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

This fixes the bug indeed; seems like we should re-visit margins regardless though.

@sirreal
Copy link
Member Author

sirreal commented Dec 3, 2018

All these styles do need to be revisited 👍

A note, the bug occurred because the observer callback set the block width, which changed the block size, which called the observer and set a different width. This appeared to manifest because the width reported to the observer did not match the width that was set on the component. This was due to the way the negative margins impacted the content rects. I'm not sure why this only manifested on the left/right alignments 🤷‍♂️

Nothing in this PR actually prevents this bug from reappearing under the right conditions, we might also look at that in a follow up.

@sirreal sirreal merged commit 9683bb0 into master Dec 3, 2018
@sirreal sirreal deleted the fix/g7g-tg-layout-renderers branch December 3, 2018 12:12
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants