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

WIP: Install a block from inserter #16524

Open
wants to merge 122 commits into
base: master
from

Conversation

@ck-lee
Copy link

commented Jul 11, 2019

Description

Extend the inserter to show available blocks for download and silently install the block when the block is inserted into the editor

This PR is still a WIP. Can you help guide me with the implementation and avoiding any pitfalls? I'm new to the block editor.

Screenshots

Types of changes

  • Extend inserter to fetch a list of suggestBlocks similar to reusableBlocks
  • The list of suggestBlocks is currently served from a mock API. It is not built yet.
  • Extend the filter to show suggestBlocks if there are no results found.
  • When "Add" button is clicked, download the block assets from plugin.svn. Then, load and insert the block into the editor without installing it.
  • ...

Relevant Links

[design] Main thread
https://make.wordpress.org/design/2019/04/26/block-library-installing-blocks-from-within-gutenberg/

[design] Figma
https://www.figma.com/file/QKhoOKXkBN2mHacqvrtdNuI6/Gutenberg-Block-Library%3A-Installation

[design] Mockup feedback
https://make.wordpress.org/design/2019/06/07/block-library-mockups-prototype

[dev] Block Directory Proposal
https://make.wordpress.org/meta/2019/03/08/the-block-directory-and-a-new-type-of-plugin/

[dev] Block Registration
https://github.com/WordPress/gutenberg/blob/master/docs/rfc/block-registration.md

[dev] Block Registration - assets JSON structure
#13693 (comment)

GitHub Block Directory issues
https://github.com/WordPress/block-directory/issues

GitHub Block Directory project
https://github.com/WordPress/block-directory/projects/1

@gziolo
Copy link
Member

left a comment

Great refactor, I left some nitpicks while doing a review of the general architecture. This is exactly what we discussed last time. I don't think I have any concerns on how it shapes. @youknowriad might need to confirm that but I guess we are in the position where we can focus on the implementation details. I'd love us to find a way to split this PR into a few incremental steps to make the process easier to share feedback.

packages/block-directory/CHANGELOG.md Outdated Show resolved Hide resolved
packages/block-directory/package.json Outdated Show resolved Hide resolved
packages/block-directory/package.json Show resolved Hide resolved
<Stars rating={ rating } />
<span
className="block-directory-block-ratings__rating-count"
aria-label={

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 13, 2019

Member

Is this aria-label necessary here? This item isn't focusable so I think this will never be announced. I see something similar applied for div later in the code. Am I missing something?

This comment has been minimized.

Copy link
@ck-lee

ck-lee Aug 18, 2019

Author

I think the aria-label may still be necessary. The screen readers can still navigate to these elements even though they are not focusable.

packages/block-directory/src/store/index.js Outdated Show resolved Hide resolved
packages/block-editor/src/store/selectors.js Outdated Show resolved Hide resolved
packages/block-editor/src/components/inserter/menu.js Outdated Show resolved Hide resolved

CK and others added some commits Aug 15, 2019

CK
Update packages/block-directory/package.json
Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>
Update packages/block-directory/package.json
Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>
Update packages/block-directory/CHANGELOG.md
Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>
CK
CK
Remove isDownloadableBlocksEnabled settings.
If no fills registered for the slot, default to original behaviour.
CK
@talldan
Copy link
Contributor

left a comment

Hey @ck-lee. I've had another look over the code. Looks good so far!

It is quite a sizeable PR so difficult to review it all thoroughly. I'm wondering if there might be a strategy for splitting this into smaller chunks and merging parts of this behind a feature flag (https://developer.wordpress.org/block-editor/developers/feature-flags/).

The slot could be a good first thing to attempt to merge in a much smaller PR.

// AMBIGUOUS: Plugin's description, not description in block.json
$block->description = wp_trim_words( wp_strip_all_tags( $plugin[ 'description' ] ), 30, '...' );
$block->rating = $plugin[ 'rating' ];

This comment has been minimized.

Copy link
@talldan

talldan Aug 20, 2019

Contributor

Looks like a minor white-space/alignment issue (with the line below).

This comment has been minimized.

Copy link
@talldan

talldan Aug 20, 2019

Contributor

Looks like there are a few linting errors in this file that have been caught by travis.


function DownloadableBlockHeader( { icon, title, rating, ratingCount, onClick } ) {
return (
<Fragment>

This comment has been minimized.

Copy link
@talldan

talldan Aug 20, 2019

Contributor

The Fragment could be removed here as everything seems to be wrapped in the div.

@@ -229,7 +231,7 @@ export class InserterMenu extends Component {
filterValue,
itemsPerCategory,
filteredItems,
reusableItems
reusableItems,

This comment has been minimized.

Copy link
@talldan

talldan Aug 20, 2019

Contributor

This is a function call, so I don't think it should have a comma at the end.

} }
>
{ ( fills ) => {
// __experimentalInserterMenuExtension should also handle when isMenuEmpty is true ( no installed block type results are found ).

This comment has been minimized.

Copy link
@talldan

talldan Aug 20, 2019

Contributor

It looks like it does, so I think this comment could be removed.

}
}
},
* hasInstallBlocksPermission() {

This comment has been minimized.

Copy link
@talldan

talldan Aug 20, 2019

Contributor

Most other permissions in the codebase are being handled by making an OPTIONS request to the rest endpoint and checking the return value of the Allow header. Could that also be an option (sorry for the pun) here?

The core-data package has a canUser selector/resolver combo for this:
https://github.com/WordPress/gutenberg/blob/master/packages/core-data/src/resolvers.js#L135

There should be a few examples of it in use across the codebase. Would be great to be able to make this work the same way.

const DOWNLOAD_ERROR_NOTICE_ID = 'block-download-error';
const INSTALL_ERROR_NOTICE_ID = 'block-install-error';

function DownloadableBlocksList( { items, onHover = () => {}, children, downloadAndInstallBlock } ) {

This comment has been minimized.

Copy link
@talldan

talldan Aug 20, 2019

Contributor

I noticed noop is already imported, so makes sense to use it here as well.

Suggested change
function DownloadableBlocksList( { items, onHover = () => {}, children, downloadAndInstallBlock } ) {
function DownloadableBlocksList( { items, onHover = noop, children, downloadAndInstallBlock } ) {
humanizedUpdated={ humanizedUpdated }
/>
</section>
<section className="block-directory-downloadable-block-list-item__footer">

This comment has been minimized.

Copy link
@talldan

talldan Aug 20, 2019

Contributor

Should this one be a footer element?

return (
<Fragment>
<span className="block-directory-downloadable-block-author-info__content-author">
Authored by <strong>{ author }</strong>

This comment has been minimized.

Copy link
@talldan

talldan Aug 20, 2019

Contributor

I'm guessing this is still WIP, but it looks like the text should be internationalized and this line is missing a period at the end

<?php
/**
* Start: Include for phase 2
* Block Areas REST API: WP_REST_Blocks_Controller class

This comment has been minimized.

Copy link
@talldan

talldan Aug 20, 2019

Contributor

The comment doesn't match the class name below.

*/
public function __construct() {
$this->namespace = '__experimental';
$this->rest_base = 'blocks';

This comment has been minimized.

Copy link
@talldan

talldan Aug 20, 2019

Contributor

AFAIK, there is already a blocks custom post type for reusable blocks, and they can be fetched from /wp/v2/blocks, so calling this endpoint the same thing might present some challenges.

installable_blocks, plugin_blocks or something like that might provide a good separation between the two.

@talldan talldan requested a review from tellthemachines Aug 20, 2019

@talldan

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Have pinged @tellthemachines for any feedback on the experiment settings page.

CK added some commits Aug 20, 2019

CK
CK
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.