Google Calendar block: Fix full width support #14835

apeatling · 2020-02-27T23:39:58Z

Make sure that the Google Calendar block widths match in the editor and front end, and that full and wide widths are supported correctly.

Fixes #14823

Testing instructions:

Add the Google Calendar block, and embed a calendar. Example:
<iframe src="https://calendar.google.com/calendar/embed?src=tl4njqffodltemv385vnifrjadm345g2%40import.calendar.google.com&ctz=America%2FVancouver" style="border: 0" width="800" height="600" frameborder="0" scrolling="no"></iframe>
Modify the alignment of the block to full width.
Save the changes and look at the block on the front end and confirm it is full width (edge to edge).
Try the left and right alignment options and confirm the width of the block matches in the front end and the editor.

Note: on some themes (e.g. Twenty Twenty) the left/right positioning of this block in the front end might overflow outside of the viewport because of its min-width. Eventually we decided that having a readable calendar on most themes is better than having a squished calendar on all themes. (See discussion in the PR comments)

Proposed changelog entry for your changes:

None

matticbot · 2020-02-27T23:40:05Z

Caution: This PR has changes that must be merged to WordPress.com
Hello apeatling! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D39574-code before merging this PR. Thank you!

scruffian · 2020-03-02T13:12:29Z

This looks good, and works as expected. However I can't see how the height attribute is used; it doesn't seem to be working. Is that expected?

apeatling · 2020-03-02T18:28:53Z

This looks good, and works as expected. However I can't see how the height attribute is used; it doesn't seem to be working. Is that expected?

It seems to set a default initial height of the iframe in the editor. I originally removed it, but in the editor the height of the iframe was always collapsed with no way to override this with CSS. It worked on the front end however.

scruffian

The height attribute seems to be working now. LGTM...

glendaviesnz

The removal of the min-width on the iframe in the editor causes the calendar to squash to a smaller width on left and right align than on centre. Dave initially picked this up here.

I vaguely remember a discussion about this, apologies if there was and I didn't pipe up then.

apeatling · 2020-03-03T21:37:09Z

@davemart-in Thoughts on the widths of left and right alignment? I do think it needs to be narrower here to allow for content to sit beside it and not also be squished:

davemart-in · 2020-03-03T21:37:26Z

@glendaviesnz here's where it was discussed previously. Originating here.

davemart-in · 2020-03-03T21:39:32Z

@apeatling We added a height property to the map block. Should we just add a width property/option in the sidebar here? That way the customer could set the width to whatever works best for their page.

jetpackbot · 2020-03-04T11:10:53Z

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.

Scheduled Jetpack release: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against 386b658

apeatling · 2020-03-04T21:09:29Z

@davemart-in We can do that, I think the issue in my mind is how does this interact with the "full-width" and "wide" alignment options? Does it just override this width setting?

davemart-in · 2020-03-04T21:35:30Z

@apeatling good question.

Would it make sense to only show the width option when the alignment is left, right, or center. Or would that be too confusing do you think?

Another option would be to fix the width to 100% when in full-width and wide mode and disable the control. Then maybe add a line under it saying "Full-width alignment applied", or something like that.

Copons · 2020-03-04T23:05:50Z

There are a couple of weird behaviours going on. 🤔

I don't understand why we set the height as block attribute, when there's no way to change it.
Might as well have it in a constant instead, and keep the block footprint lighter?

How the heck is the iframe width calculated when float aligned? 😮
It seems to be always 260px to me...
I've tried adding a width="600" to the iframe in the PHP side, and not only it was still rendered 260px wide, but it also changed the height to 260! 😅
Changing the container's width doesn't seem to do anything as well...

glendaviesnz · 2020-03-05T17:19:58Z

How the heck is the iframe width calculated when float aligned? 😮

Yeh, I couldn't for the life of me work out where that float width was being inherited from, but this does override it - 1334560#diff-6c3f07277cda5eda687f141e3ef5e805

glendaviesnz · 2020-03-05T17:26:22Z

I don't understand why we set the height as block attribute, when there's no way to change it.

It is extracted from the embed code as the user can specify custom height and width when setting up the embed - but we should also have an option for the user to change this once the block is added.

apeatling · 2020-03-05T17:26:55Z

I think we can add back the min width only when the block is aligned left and right. I pretty skeptical that adding a width control is a worthy time investment though. I think we could just set the min width to 50% when aligned left/right, and support full/wide width and move on. If we hear from users that they want to change this, then look at more fine grained control.

apeatling · 2020-03-05T17:54:51Z

All of the default themes provide default styles for alignleft and alignright floats that causes this to be really problematic. Not to mention themes like TwentyTwenty put floats in their own column with a restricted width.

scruffian · 2020-03-05T21:21:01Z

This opens of the question of how much control we should give users to do things that might not always work well. Its hard to know where to draw the line between giving them the freedom to break stuff and preventing them from creating bad designs.

matticbot · 2020-03-10T16:55:26Z

apeatling, Your synced wpcom patch D39574-code has been updated.

github-actions · 2020-03-10T16:56:46Z

Howdy! The Jetpack team has disappeared for a few days to a secret island lair to concoct new ways to make Jetpack one hundred billion percent better. As a result, your Pull Request may not be reviewed right away. Do not worry, we will be back next week to look at your work! Thank you for your understanding.

davemart-in · 2020-03-10T20:03:31Z

I think we can add back the min width only when the block is aligned left and right.

I'd vote to do this and then call it good.

Copons · 2020-03-11T11:30:24Z

I think we can add back the min width only when the block is aligned left and right.

I'd vote to do this and then call it good.

@davemart-in It was unclear in my previous comment, but I believe this to be very hard to develop.
The problem is that themes treat floaty-aligned blocks in unpredictable ways: some move them outside of the content, some resize them responsively, some calculate the position from the content width, etc.
We can add a min width to the block, but we might break some themes in the process. 😞

davemart-in · 2020-03-11T13:53:43Z

We can add a min width to the block, but we might break some themes in the process. 😞

The alternative is that the calendar will look broken by default on ALL themes when the alignment is set to left or right.

This calendar just looks broken to me. I can't imagine that anyone would want this to show up like this on their site.

Themes can always override the min-width if they want.

Copons · 2020-03-11T14:16:40Z

@davemart-in 🤔 makes sense.

I've tried with min-width: 420px (which is roughly the size where an event popup is fully visible in the embed), and this is how it looks at different sizes on Twenty Nineteen and Twenty Twenty:

1920	1280	700

Unfortunately Twenty Twenty's float alignment is 290px wide, so at some screen sizes the calendar would end up outside, but there's not much we can do there.
I think this is good enough though, should I commit?

davemart-in · 2020-03-11T16:28:56Z

Aside: Odd that there would be more text content visible in the 700px resolution vs. the 1280px resolution:

I think this is good enough though, should I commit?

I think this is better than the alternative of not setting a min-width, so yes, let's commit it.

Thank you!

matticbot · 2020-03-11T17:59:53Z

apeatling, Your synced wpcom patch D39574-code has been updated.

apeatling · 2020-03-12T16:55:59Z

@Copons Thanks for digging into and following up on this one! 🥇

apeatling · 2020-03-16T15:52:47Z

@Copons With your commit I think this is ready for final review, right?

Copons · 2020-03-16T17:52:07Z

@Copons With your commit I think this is ready for final review, right?

@apeatling Yes I believe so

scruffian

Let's merge this 🚢

jeherve

This works well for me, it should be good to merge!

Lots changed since this review 🙂

* Initial changelog entry * Changelog: add #14904 * Changelog: add #14910 * Changelog: add #14913 * Changelog: add #14916 * Changelog: add #14922 * Changelog: add #14924 * Changelog: add #14925 * Changelog: add #14928 * Changelog: add #14840 * Changelog: add #14841 * Changelog: add #14842 * Changelog: add #14826 * Changelog: add #14835 * Changelog: add #14859 * Changelog: add #14884 * Changelog: add #14888 * Changelog: add #14817 * Changelog: add #14814 * Changelog: add #14819 * Changelog;: add #14797 * Changelog: add #14798 * Changelog: add #14802 * Changelog: add #13676 * Changelog: add #13744 * Changelog: add #13777 * Changelog: add #14446 * Changelog: add #14739 * Changelog: add #14770 * Changelog: add #14784 * Changelog: add #14897 * Changelog: add #14898 * Changelog: add #14968 * Changelog: add #14985 * Changelog: add #15044 * Changelog: add #15052 * Update to remove Podcast since it remains in Beta * Changelog: add #14803 * Changelog: add #15028 * Changelog: add #15065 * Changelog:add #14886 * Changelog: add #15118 * Changelog: add #14990 * Changelog: add #14528 * Changelog: add #15120 * Changelog: add #15126 * Changelog: add #15049 * Chanegelog: add #14852 * Changelog: add #15090 * Changelog: add #15138 * Changelog: add #15124 * Changelog:add #15055 * Changelog: add #15017 * Changelog: add #15109 * Changelog: add #15145 * Changelog:add #15096 * Changelog:add #15153 * Changelog: add #15133 * Changelog: add #14960 * Changelog: add #15127 * Changelog: add #15056 * Copy current changelog to changelog archive. * Clarify changelog description

westonruter · 2020-09-24T04:03:41Z

extensions/blocks/google-calendar/google-calendar.php

 	if ( empty( $url ) ) {
 		return;
 	}

 	if ( class_exists( 'Jetpack_AMP_Support' ) && \Jetpack_AMP_Support::is_amp_request() ) {
 		return sprintf(
-			'<div class="%1$s"><amp-iframe src="%2$s" frameborder="0" style="border:0" scrolling="no" width="%3$d" height="%4$d" sandbox="allow-scripts allow-same-origin" layout="responsive"></amp-iframe></div>',
+			'<div class="%1$s"><amp-iframe src="%2$s" frameborder="0" style="border:0" scrolling="no" height="%3$d" sandbox="allow-scripts allow-same-origin" layout="responsive"></amp-iframe></div>',


This change appears to have broken Google Calendar on AMP pages, since there is no width defined, and the layout was not changed from responsive to fixed-height. Opened PR to fix in #17251.

apeatling added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Feb 27, 2020

apeatling requested review from pento and a team February 27, 2020 23:39

apeatling self-assigned this Feb 27, 2020

matticbot added the Touches WP.com Files label Feb 27, 2020

apeatling added the [Status] Needs Cherry-Pick label Feb 27, 2020

apeatling added this to the 8.4 milestone Feb 28, 2020

scruffian previously approved these changes Mar 3, 2020

View reviewed changes

glendaviesnz previously requested changes Mar 3, 2020

View reviewed changes

Copons added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Mar 10, 2020

Copons requested a review from glendaviesnz March 10, 2020 16:56

Add a min-width to the block to avoid squishing it when floating

386b658

scruffian approved these changes Mar 17, 2020

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Mar 17, 2020

jeherve approved these changes Mar 17, 2020

View reviewed changes

Copons merged commit c163451 into master Mar 18, 2020

Copons deleted the fix/google-calendar-full-width branch March 18, 2020 11:33

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 18, 2020

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Mar 20, 2020

jeherve added a commit that referenced this pull request Mar 20, 2020

Changelog: add #14835

10185fa

westonruter mentioned this pull request Sep 24, 2020

Fix AMP compatibility for Google Calendar block #17251

Merged

westonruter reviewed Sep 24, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Google Calendar block: Fix full width support #14835

Google Calendar block: Fix full width support #14835

apeatling commented Feb 27, 2020 •

edited by Copons

Loading

matticbot commented Feb 27, 2020

scruffian commented Mar 2, 2020

apeatling commented Mar 2, 2020 •

edited

Loading

scruffian left a comment

glendaviesnz left a comment •

edited

Loading

apeatling commented Mar 3, 2020

davemart-in commented Mar 3, 2020

davemart-in commented Mar 3, 2020

jetpackbot commented Mar 4, 2020 •

edited

Loading

apeatling commented Mar 4, 2020

davemart-in commented Mar 4, 2020

Copons commented Mar 4, 2020

glendaviesnz commented Mar 5, 2020

glendaviesnz commented Mar 5, 2020

apeatling commented Mar 5, 2020 •

edited

Loading

apeatling commented Mar 5, 2020

scruffian commented Mar 5, 2020

matticbot commented Mar 10, 2020

github-actions bot commented Mar 10, 2020

davemart-in commented Mar 10, 2020

Copons commented Mar 11, 2020

davemart-in commented Mar 11, 2020

Copons commented Mar 11, 2020

davemart-in commented Mar 11, 2020

matticbot commented Mar 11, 2020

apeatling commented Mar 12, 2020

apeatling commented Mar 16, 2020

Copons commented Mar 16, 2020

scruffian left a comment

jeherve left a comment

westonruter Sep 24, 2020

Google Calendar block: Fix full width support #14835

Google Calendar block: Fix full width support #14835

Conversation

apeatling commented Feb 27, 2020 • edited by Copons Loading

Testing instructions:

Proposed changelog entry for your changes:

matticbot commented Feb 27, 2020

scruffian commented Mar 2, 2020

apeatling commented Mar 2, 2020 • edited Loading

scruffian left a comment

Choose a reason for hiding this comment

glendaviesnz left a comment • edited Loading

Choose a reason for hiding this comment

apeatling commented Mar 3, 2020

davemart-in commented Mar 3, 2020

davemart-in commented Mar 3, 2020

jetpackbot commented Mar 4, 2020 • edited Loading

apeatling commented Mar 4, 2020

davemart-in commented Mar 4, 2020

Copons commented Mar 4, 2020

glendaviesnz commented Mar 5, 2020

glendaviesnz commented Mar 5, 2020

apeatling commented Mar 5, 2020 • edited Loading

apeatling commented Mar 5, 2020

scruffian commented Mar 5, 2020

matticbot commented Mar 10, 2020

github-actions bot commented Mar 10, 2020

davemart-in commented Mar 10, 2020

Copons commented Mar 11, 2020

davemart-in commented Mar 11, 2020

Copons commented Mar 11, 2020

davemart-in commented Mar 11, 2020

matticbot commented Mar 11, 2020

apeatling commented Mar 12, 2020

apeatling commented Mar 16, 2020

Copons commented Mar 16, 2020

scruffian left a comment

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

westonruter Sep 24, 2020

Choose a reason for hiding this comment

apeatling commented Feb 27, 2020 •

edited by Copons

Loading

apeatling commented Mar 2, 2020 •

edited

Loading

glendaviesnz left a comment •

edited

Loading

jetpackbot commented Mar 4, 2020 •

edited

Loading

apeatling commented Mar 5, 2020 •

edited

Loading