-
Notifications
You must be signed in to change notification settings - Fork 798
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
Blocks: Update placeholders to use the withNotices HOC #14884
Conversation
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: April 7, 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: WordPress 5.3 looks like this: 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. |
Should we also update the Eventbrite and Pinterest blocks? We should also update the block scaffolding... |
Done in #14895 |
Tracking this in #14896 |
'jetpack' | ||
) } | ||
</> | ||
const setErrorNotice = () => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! 👍
Caution: This PR has changes that must be merged to WordPress.com |
r203925-wpcom |
* 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
Changes proposed in this Pull Request:
withNotices
HOC instead of manually handling the error notices.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 anoticeUI
prop that is passed to the Placeholder through thenotices
prop.E.g.
Some blocks instead use the
noticeUI
outside the Placeholder (the notice would appear above the block content—but still inside its container), while passing anotices
prop to the Placeholdernotices
prop.E.g.
jetpack/extensions/blocks/map/edit.js
Line 399 in 0fd1874
jetpack/extensions/blocks/map/edit.js
Line 492 in 0fd1874
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:
Proposed changelog entry for your changes:
withNotices
higher-order component.