Block: OpenTable: Add a warning if width is set to anything other than wide if style is wide

[glendaviesnz]

Fixes #14857

Currently the OpenTable wide style does not display well in themes if the block alignment is set to anything other than wide also. If a user selects the wide style the alignment is automatically set back to wide, but the user can change this using the block alignment toolbar option.

There is currently no way to disable particular options in the alignment toolbar on the fly, so this PR instead adds a warning notice to at least alert the user to the fact that their current style/align combination may cause display issues.

Changes proposed in this Pull Request:

• Adds a warning notice if a user selects a style/align combination that may cause display issues

Testing instructions:

Add this PR to your local Jetpack dev env
Add an OpenTable block
Try setting the block style to wide, and then set the block alignment to something other than wide
Make sure the warning notice appears and is dismissible
Make sure the notice doesn't appear on page reload of those alignment settings are saved
Make sure the notice disappears if the alignment is set back to wide

Before:

After:

Proposed changelog entry for your changes:

• Warning notice added to the OpenTable block to alert the user when their selected combination of style and align options may cause display issues

[matticbot]

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

[jetpackbot]

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: May 5, 2020.
Scheduled code freeze: April 28, 2020

Generated by 🚫 dangerJS against 5ae7066

[glendaviesnz]

The only slight issue with this approach is the notice layout is squashed up when left or right align is chosen:

I played around with some absolute position to try and get around this, but that caused more layout issues. This is bit of an edge case and the notice is dismissible, so probably not too big an issue.

[Copons]

Is there a reason for this? I'd rather show the warning all the time (when style is wide and align isn't), so that the user is always aware of it.
Otherwise, they might save the post, come back later and have no idea why it looks wonky.

[glendaviesnz]

I was just assuming that if they decided to ignore the warning and save it, then they had a reason for doing so, so didn't wanted to be reminded every time they loaded the page ... but you maybe you are correct and we should hassle them every time as it will obviously be broken, so have taken this out for now.

[Copons]

This needs a few more checks!

• The notice shouldn't appear on the wide style preview in the sidebar.
• The notice shouldn't appear on the placeholder, where we already use noticeUI for embed errors.

I've had success with this:

const { noticeOperations } = props;
const { __isBlockPreview, align, rid, style } = attributes;

useEffect( () => {
	noticeOperations.removeAllNotices();
	if ( ! isEmpty( rid ) && ! __isBlockPreview && 'wide' === style && 'wide' !== align ) {
		noticeOperations.createNotice( {
			status: 'warning',
			content: __( 'Warning' ),
		} );
	}
}, [ __isBlockPreview, align, noticeOperations, rid, style ] );

Notes:

• isEmpty( rid ) is what we use for determining if the block is in placeholder state. Since we use it a few times, we might as well assign it to a more clear variable, e.g. const isPlaceholder = empty( rid ).
• __isBlockPreview is part of our own BlockStylesSelector component, and turns out to be super handy in this case!
• We need to pass the actual used variables to the hook, not just the attributes object, or we would trigger this every time any attribute changes.

[glendaviesnz]

Ahh, thanks for all this extra detail ... I still have a lot to learn about the jetpack blocks, been stuck in the back end for too long! I have made all these changes and seems much more robust now.

[Copons]

The block is styled in a bit of a complicated way, with an overlay setting its height.
If we render the notice here, the embed would overflow outside the overlay.
You can observe this in one of the PR gifs, where the embed moves behind the inserter button.

I've tried a few options, and had success with moving it outside the edit wrapper.

<>
	{ noticeUI }
	<div className={ editClasses }>
		{ ! isEmpty( rid ) && inspectorControls }
		{ ! isEmpty( rid ) ? blockPreview() : blockPlaceholder }
	</div>
</>

Awkwardly enough, when the block is centrally aligned, the text of the notice is centered as well.
We might want to fix it in CSS. 🤔

[glendaviesnz]

Yep, this is much gooder thanks ... fixed the centre alignment too.

[matticbot]

glendaviesnz, Your synced wpcom patch D41472-code has been updated.

[matticbot]

glendaviesnz, Your synced wpcom patch D41472-code has been updated.

[matticbot]

glendaviesnz, Your synced wpcom patch D41472-code has been updated.

[glendaviesnz]

thanks @Copons, ready for another review I think.

[matticbot]

glendaviesnz, Your synced wpcom patch D41472-code has been updated.

[Copons]

Thanks for the updates @glendaviesnz!
I took the liberty to push some ESLint fixes, as IIRC you had some issues going on there. I think it should be enough to yarn install, as there have been some recent changes there.

Anyway, while testing this again, I've noticed that the warning shows up with align="full" as well.
At first I was like: well, let's add 'full' === align to the conditions!
But then I'm wondering: the issue here is that the embed has a fixed width of 840px, and so we are asking people to use it in containers as large as possible.
wide and full are usually larger than 840px, but not always.
For example, a theme might define wide as narrower. Most importantly, both alignments are generally responsive, and I'm pretty sure the wide widget would look bad on mobile.

We have limited powers over the embed, but I guess there might some things we can do here:

• Add 'full' === align to the "approved" conditions (and update the warning accordingly).
• Add a note to the warning saying that wide widgets might look bad on small screens.
• Add an overflow: auto to the embed container, so that the widget would be half-usable on small screens as well.

What do you think? 🤔

[matticbot]

glendaviesnz, Your synced wpcom patch D41472-code has been updated.

[glendaviesnz]

@Copons - thanks for fixing the lint issues - I have the pre-commit hooks working again, so hopefully less of those now!

Nice catch on the full width - have added support for that, and also updated the message (which probably now should actually say, "don't use the wide style cause it sucks, it is only there so we have an even number of styles to show!").

I tried setting overflow to auto, but this only worked on 'centre' align for some reason that I was unable to work out. Feel free to have go at this and update the PR if you get it working.

[Copons]

I couldn't figure out a way to show scrollbars either. I think it's something that needs to be done inside the iframe. 😞

Regardless, this LGTM!

[jeherve]

This works well in my tests. 🚢