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

Slideshow Block: Fix Wide/Full Alignment #17645

Merged
merged 1 commit into from
Oct 29, 2020

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Oct 28, 2020

Fixes #17616. Carried over from WordPress/gutenberg#26552 so we have a short-term fix while waiting for that PR to land.

Changes proposed in this Pull Request:

Set max-width of .interface-interface-skeleton__editor to 100%, so that blocks cannot

exceed the editor boundaries and, when set to full width, gain a width of literally millions of pixels, making the editor unusable.

For more details, see WordPress/gutenberg#26552.

Jetpack product discussion

N/A

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

Verify that #17616 is fixed.

Proposed changelog entry for your changes:

Set max-width of .interface-interface-skeleton__editor to 100% to fix wide/full alignment of Slideshow Block.

@ockham ockham added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Pri] High [Block] Slideshow labels Oct 28, 2020
@ockham ockham added this to the 9.1 milestone Oct 28, 2020
@ockham ockham requested review from a team October 28, 2020 22:00
@ockham ockham self-assigned this Oct 28, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello ockham! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D51956-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@jetpackbot
Copy link
Collaborator

Scheduled Jetpack release: November 10, 2020.
Scheduled code freeze: November 3, 2020

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17645

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Generated by 🚫 dangerJS against fa3b674

@ockham
Copy link
Contributor Author

ockham commented Oct 28, 2020

Note: Let's add an e2e test to cover the Slideshow's full/wide alignment options to make sure we'd catch any regressions in the future.

Copy link
Contributor

@fullofcaffeine fullofcaffeine left a comment

Choose a reason for hiding this comment

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

Tested in a local Jetpack instance with GB 9.2.2. Without this patch, the reported error happens. With this patch, setting the block to full-width works as expected ✔️

@jeyip
Copy link
Contributor

jeyip commented Oct 28, 2020

Tests

Tested using local jetpack dev environment

Functionality

  • The slideshow block does not disappear when selecting the full width setting
  • The slideshow block full width setting behaves as expected
  • Full width frontend block behaves as expected

Browsers

  • Chrome
  • Firefox
  • Edge
  • Safari

Copy link
Contributor

@jeyip jeyip left a comment

Choose a reason for hiding this comment

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

I can confirm that this fixes the issue and doesn't appear to cause regressions in the interface layout or block width settings

@fullofcaffeine
Copy link
Contributor

Thanks @jeyip!

@ockham
Copy link
Contributor Author

ockham commented Oct 28, 2020

Thanks Marcelo and Jeremy!

I'll go ahead and merge the WP.com counterpart, since we're seeing a steady influx of users running into this issue.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! [Status] Tested on WP.com labels Oct 29, 2020
@jeherve
Copy link
Member

jeherve commented Oct 29, 2020

r216021-wpcom

@jeherve jeherve merged commit f081fe3 into master Oct 29, 2020
@jeherve jeherve deleted the fix/slideshow-block-fullscreen-mode branch October 29, 2020 07:24
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 29, 2020
@jeherve
Copy link
Member

jeherve commented Oct 29, 2020

Cherry-picked to branch-9.1 in f63e377

@Copons
Copy link
Contributor

Copons commented Dec 9, 2020

Hi @Automattic/good-mountain, I've got bad news!

WordPress/gutenberg#27622

@sirreal
Copy link
Member

sirreal commented Dec 10, 2020

Thanks for flagging this @Copons.

I'm looking at my Atomic site with Gutenberg 9.5.1 and Jetpack 9.2 and it seems to include the fix.

Do we need to revert be7ab6e6f9952c815db59024eefd7b8af4e5411d before the next release?

CC: @jeherve it looks like the Gutenberg-side fix for this regressed in 9.5.

@Copons
Copy link
Contributor

Copons commented Dec 10, 2020

Thanks for looking into this y'all!

@sirreal you're right, the fix is included in 9.2 and the max-width now shows up for me on the latest master 🤔
I guess I was on an old/wonky JP master when I tested.

Also... I can't find where be7ab6e was merged... 😮
GH says:

This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

So maybe we don't need to do anything JP-side?

@jeherve
Copy link
Member

jeherve commented Dec 10, 2020

Yeah, I think we're all set. That commit is an orphan, I think it was created while testing this branch.

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

Successfully merging this pull request may close these issues.

Slideshow Block: Block breaks in Editor when set to Full Width
8 participants