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

Update block scaffolding to use the withNotices HOC #14895

Merged
merged 2 commits into from
Mar 5, 2020

Conversation

scruffian
Copy link
Member

In of #14884 we realised we were doing notices wrongly. We should update the block scaffolding to do it the right way.

Changes proposed in this Pull Request:

  • Update the block scaffolding to use the withNotices HOC

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • change to existing feature

Testing instructions:

  • Run yarn docker:wp jetpack scaffold block "Cool block"
  • Open the editor
  • Try adding a "Cool block" to the editor.
  • Check that the block loads correctly

Proposed changelog entry for your changes:

  • no changelog

@scruffian scruffian added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Mar 5, 2020
@scruffian scruffian requested a review from a team March 5, 2020 13:00
@scruffian scruffian requested a review from a team as a code owner March 5, 2020 13:00
@scruffian scruffian self-assigned this Mar 5, 2020
@jetpackbot
Copy link

jetpackbot commented Mar 5, 2020

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 4bee796

extensions/index.json Outdated Show resolved Hide resolved
Comment on lines +24 to +27
const setErrorNotice = () => {
noticeOperations.removeAllNotices();
noticeOperations.createErrorNotice( __( 'Put error message here.', 'jetpack' ) );
};
Copy link
Member

Choose a reason for hiding this comment

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

Part of me wonders if we shouldn't extract this into a shared function available to all blocks? Ideally, that function could also handle all kinds of notices, not just erors. Something like this:

export function displayNotice( message, noticeOperations, type = 'error' ) {
	noticeOperations.removeAllNotices();
	noticeOperations.createNotice( { status: type, content: message } );
}

Copy link
Member Author

Choose a reason for hiding this comment

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

That might be worth trying in some blocks, but I'd rather we only move stuff into the scaffolding when we've already had success with it in other blocks.

Copy link
Member

Choose a reason for hiding this comment

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

You're right. I suggested we give that a try in #14884 (comment)

Until then, we should be able to merge your PR and revisit once we have some blocks using it.

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Mar 5, 2020
Co-Authored-By: Jeremy Herve <jeremy@jeremy.hu>
@jeherve jeherve added this to the 8.4 milestone Mar 5, 2020
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! [Type] Janitorial 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 5, 2020
@scruffian scruffian merged commit 3627e15 into master Mar 5, 2020
@scruffian scruffian deleted the update/block-scaffolding-notices branch March 5, 2020 16:31
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Type] Janitorial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants