Skip to content

Blocks: Update placeholders to use the withNotices HOC#14884

Merged
Copons merged 4 commits intomasterfrom
update/blocks-placeholders-with-notices
Mar 5, 2020
Merged

Blocks: Update placeholders to use the withNotices HOC#14884
Copons merged 4 commits intomasterfrom
update/blocks-placeholders-with-notices

Conversation

@Copons
Copy link
Copy Markdown
Contributor

@Copons Copons commented Mar 4, 2020

Changes proposed in this Pull Request:

  • Update the Amazon, Calendly, Google Calendar, and OpenTable blocks to use the withNotices HOC instead of manually handling the error notices.

Screenshot 2020-03-04 at 13 12 20

Pros: it's the Core Gutenberg way of handling placeholder notices; manual notices are incorrectly positioned on recent Gutenberg versions (see #14833 for a CSS-only fix).

Cons: notices created with withNotices are always dismissable, while all our manual notices aren't. I don't think it's a big deal to be honest.

Note

Other blocks already use the withNotices HOC, but some in a slightly peculiar way.
According to the Gutenberg docs, withNotices exposes a noticeUI prop that is passed to the Placeholder through the notices prop.
E.g.

<Placeholder notices={ props.noticeUI } />

Some blocks instead use the noticeUI outside the Placeholder (the notice would appear above the block content—but still inside its container), while passing a notices prop to the Placeholder notices prop.
E.g.

<Fragment>
  { props.noticeUI }
  <Placeholder notices={ props.notices } />
</Fragment>

notices={ notices }

I'm not exactly sure why this happens, and for example why this doesn't end up displaying two notices, one in the Placeholder, and one above the block content.
I've chosen to not touch these blocks, to avoid unexpected regressions.

Testing instructions:

  • Insert the Amazon, Calendly, Google Calendar, and OpenTable blocks, and try triggering an error notice by inserting an incorrect URL or embed code.
  • Make sure the notice still works and is positioned consistently.

Proposed changelog entry for your changes:

  • Update the Amazon, Calendly, Google Calendar, and OpenTable blocks to use the withNotices higher-order component.

@Copons Copons added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review This PR is ready for review. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] OpenTable [Block] Calendly [Block] Amazon [Block] Google Calendar labels Mar 4, 2020
@Copons Copons requested review from a team March 4, 2020 19:14
@Copons Copons self-assigned this Mar 4, 2020
@jetpackbot
Copy link
Copy Markdown
Collaborator

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 078f7cf

@pablinos
Copy link
Copy Markdown
Contributor

pablinos commented Mar 5, 2020

This looks good to me. I forgot to take a screenshot, but with Gutenberg 7.3 and WordPress 5.2 the notice was still indented. That's an odd combination though.

Vanilla WordPress 5.2 looks like this:

image

WordPress 5.3 looks like this:

image

And with the latest Gutenberg, it looks as in the screenshots above.

These all seem acceptable to me, and the indentation I was seeing with an older version of Gutenberg is probably not an issue.

@scruffian
Copy link
Copy Markdown
Member

Should we also update the Eventbrite and Pinterest blocks?

We should also update the block scaffolding...

@scruffian
Copy link
Copy Markdown
Member

We should also update the block scaffolding...

Done in #14895

@scruffian
Copy link
Copy Markdown
Member

Should we also update the Eventbrite and Pinterest blocks?

Tracking this in #14896

'jetpack'
) }
</>
const setErrorNotice = () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As I noted here, I wonder if we should extract this into a shared function maybe?

#14895 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is a good idea, but lets do it in another PR. I have created an issue to track it here: #14899

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Mar 5, 2020
@scruffian scruffian added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 5, 2020
@jeherve jeherve removed the [Status] Needs Review This PR is ready for review. label Mar 5, 2020
@jeherve jeherve added the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 5, 2020
@jeherve jeherve added this to the 8.4 milestone Mar 5, 2020
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good! 👍

@matticbot
Copy link
Copy Markdown
Contributor

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

@Copons Copons merged commit 76ab061 into master Mar 5, 2020
@Copons Copons deleted the update/blocks-placeholders-with-notices branch March 5, 2020 17:54
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 5, 2020
@Copons
Copy link
Copy Markdown
Contributor Author

Copons commented Mar 5, 2020

r203925-wpcom

jeherve added a commit that referenced this pull request Mar 20, 2020
jeherve added a commit that referenced this pull request Mar 31, 2020
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Block] Amazon [Block] Calendly [Block] Google Calendar [Block] OpenTable Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants