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

[RNMobile] Add Reusable blocks to the inserter menu #25383

Closed
wants to merge 48 commits into from
Closed

[RNMobile] Add Reusable blocks to the inserter menu #25383

wants to merge 48 commits into from

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Sep 16, 2020

Gutenberg-mobile PR -> wordpress-mobile/gutenberg-mobile#2644

Description

This PR includes the changes from #25265

Add support to insert existing Reusable blocks from the editor in the mobile version. For this purpose the inserter menu now could show 2 tabs in case this kind of blocks have been previously created in the site.

The Reusable blocks tab will show all the blocks created of this type in the site.

How has this been tested?

I tested this feature in the following 3 different scenarios:

WordPress.com site

Create a WordPress.com site or use an already created one.

  • Create a post in the web version
  • Add some blocks and create a Reusable block
  • Open the app with metro running (Gutenberg-mobile)
  • Add the WordPress.com site
  • Create a new post or Edit a post
  • Tap on the + icon to pop up the inserter menu
  • Check that 2 tabs are shown, one (first one to be presented) with the blocks and another one with Reusable blocks
  • Check that there're no Reusable blocks in the blocks tab
  • Navigate to Reusable blocks tab
  • Check that previously created Reusable blocks are shown
Self-hosted site with Jetpack

Create a self-hosted site with Jetpack (I used https://jurassic.ninja/create/) or use an already created one.

  • Create a post in the web version
  • Add some blocks and create a Reusable block
  • Open the app with metro running (Gutenberg-mobile)
  • Add the self-hosted site
  • Create a new post or Edit a post
  • Tap on the + icon to pop up the inserter menu
  • Check that 2 tabs are shown, one (first one to be presented) with the blocks and another one with Reusable blocks
  • Check that there're no Reusable blocks in the blocks tab
  • Navigate to Reusable blocks tab
  • Check that previously created Reusable blocks are shown
Self-hosted site without Jetpack

Create a self-hosted site without Jetpack (I created a local one using this instructions) or use your own.

  • Create a post in the web version
  • Add some blocks and create a Reusable block
  • Open the app with metro running (Gutenberg-mobile)
  • Add the self-hosted site
  • Create a new post or Edit a post
  • Tap on the + icon to pop up the inserter menu
  • Check that 2 tabs are shown, one (first one to be presented) with the blocks and another one with Reusable blocks
  • Check that there're no Reusable blocks in the blocks tab
  • Navigate to Reusable blocks tab
  • Check that previously created Reusable blocks are shown

Screenshots

iOS Screenshots

iPhone 8

iPhone8-inserter-menu

Blocks tab Reusable blocks tab No reusable blocks created
Captura de pantalla 2020-09-16 a las 20 48 57 Captura de pantalla 2020-09-16 a las 20 49 02 Captura de pantalla 2020-09-16 a las 21 19 06
Android Screenshots

Nexus 5

nexus5-inserter-menu

Blocks tab Reusable blocks tab No reusable blocks created
Captura de pantalla 2020-09-16 a las 21 30 43 Captura de pantalla 2020-09-16 a las 21 32 26 Captura de pantalla 2020-09-16 a las 21 46 59

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

Update `master` branch from original repo.
Comment on lines 65 to 69
const perPage = Platform.select( {
// Unbounded queries don't work on native so we can only fetch the maximum items for now
native: 100,
web: -1,
} );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I checked if per_page=-1 query parameter is included in the request, one of the api-fetch middleware items (fetch-all-middleware) consider it as an unbounded query. This means that it will try to fetch all the available items of this resource by potentially doing multiple requests with the maximum items per page until it reaches the end.

As stated in the code:

// The REST API enforces an upper limit on the per_page option. To handle large
// collections, apiFetch consumers can pass `per_page=-1`; this middleware will
// then recursively assemble a full response array from all available pages.

The problem is that this middleware requires the full response object to apply its logic, for example it needs to read the link header to know the next page. Unfortunately the mobile version of api-fetch doesn't return the full response, it only returns the response's body so we can't rely on this logic, which means that we can't use that query parameter because it raises an exception.

My first approach was to find a workaround in the Gutenberg project but for the mobile version the request logic is handled in the native side, maybe in the future that part should be reviewed because this could be a limitation if we need to do unbounded queries.

For now, as a workaround, what I've done is fetch the maximum items per page that should be enough for Reusable blocks.

Copy link
Contributor Author

@fluiddot fluiddot Sep 21, 2020

Choose a reason for hiding this comment

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

I also wrote (internal reference p9ugOq-1in-p2) about this just in case we want to tackle it in the future.

</>
);
const getHeader = () =>
header || (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This component only allowed for the header a text, this new prop will allows to add anything we might need to render in the header.

@@ -75,6 +76,7 @@ export { default as Gradient } from './mobile/gradient';
export { default as ColorSettings } from './mobile/color-settings';
export { LinkPicker } from './mobile/link-picker';
export { default as LinkSettings } from './mobile/link-settings';
export { default as SegmentedControl } from './mobile/segmented-control';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surprisingly this component was not exported I saw that it was only used in another component).

Update master branch from original repo
Update master branch from original repo.
Update master branch from original repo
@fluiddot
Copy link
Contributor Author

fluiddot commented Oct 2, 2020

Here I'm resuming the pending things to do in this PR:

  • This PR hasn't been reviewed so that would be the first thing and depending on the feedback apply some changes.

Apart from the feedback this PR was ready.

Update master branch from original repo.
Update master branch from original repo
Update master branch from original repo
The value is now the same as the missing component since they behave the same, this way the UI tests can follow the same flow.
Block name's field was being used which is not unique so it has been changed to id field.
@fluiddot fluiddot added the [Status] Duplicate Used to indicate that a current issue matches an existing one and can be closed label Jan 26, 2021
@fluiddot fluiddot self-assigned this Jan 26, 2021
@fluiddot
Copy link
Contributor Author

The work on this PR will continue in #28495.

@fluiddot fluiddot closed this Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Status] Duplicate Used to indicate that a current issue matches an existing one and can be closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants