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] Reusable blocks - Present block content (preview mode) #25265

Merged
merged 46 commits into from
Jan 21, 2021
Merged

[RNMobile] Reusable blocks - Present block content (preview mode) #25265

merged 46 commits into from
Jan 21, 2021

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Sep 11, 2020

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

Second part (adding Reusable blocks to the inserter menu):
Gutenberg PR -> #25383
Gutenberg-mobile PR -> wordpress-mobile/gutenberg-mobile#2644

Description

This PR is the first part of the issue Add Support to Insert Existing Reusable Blocks from the Editor.

The scope of this issue is limited to add Reusable blocks to the inserter menu, however due to the need of registering the Reusable block type (core/block), I also had to add support for presenting it in the editor.

The edition support will be added in this issue, so for now this PR only provides the preview mode of this type of block.

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
  • Edit the previous created post
  • Check that the Reusable block is shown
  • Tap on it and check that the title of the Reusable blocks is 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
  • Edit the previous created post
  • Check that the Reusable block is shown
  • Tap on it and check that the title of the Reusable blocks is 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
  • Edit the previous created post
  • Check that the Reusable block is shown
  • Tap on it and check that the title of the Reusable blocks is shown

Screenshots

iOS Screenshots

iPhone 8

ios-reusable-block

Reusable block unselected Reusable block selected
Android Screenshots

Nexus 5

android-reusable-block

Reusable block unselected Reusable block selected
Unsupported Block Editor support - Self-hosted site
Without Jetpack With Jetpack

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.

@fluiddot fluiddot marked this pull request as ready for review September 16, 2020 18:02
@guarani guarani added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) Needs Design Feedback Needs general design feedback. labels Sep 16, 2020
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

I gave this a quick test using the WPiOS app as you described in the test steps. Reusable blocks that I'd created on the web were visible, that's looking really solid 👍. Also, the block inserter showed my reusable block and I was able to add more instances of it to the post.

I have a couple of questions:

  1. On a post with an existing reusable block, I can add more instances of that reusable block via the block inserter. However, if a new post is opened from within WPiOS, I don't see my reusable block — was that left out of scope of this PR in favor of [RNMobile] Add Reusable blocks to the inserter menu #25383? I reread the PR description but I'm not clear on this.
  2. We've talked offline about reusing code before, but it was in the context of a scrollable list of elements. Looking at packages/block-library/src/block/edit.native.js, I see there is a lot of code that is the same between the native and web versions. Would it make sense to share more code here?

@guarani
Copy link
Contributor

guarani commented Sep 17, 2020

Gutenberg on mobile doesn't support the all core blocks, so it's possible to have a reusable block that's composed of some "unsupported blocks". While testing this scenario, I could see that the "Unsupported" placeholder block shows up correctly 👍 — but I'm not sure if the (?) button should be hidden. Here's a picture:

See screenshot Screen Shot 2020-09-17 at 10 29 42

Wdyt @fluiddot? Maybe this needs some design input (cc @iamthomasbishop).

@fluiddot
Copy link
Contributor Author

I gave this a quick test using the WPiOS app as you described in the test steps. Reusable blocks that I'd created on the web were visible, that's looking really solid 👍. Also, the block inserter showed my reusable block and I was able to add more instances of it to the post.

I have a couple of questions:

  1. On a post with an existing reusable block, I can add more instances of that reusable block via the block inserter. However, if a new post is opened from within WPiOS, I don't see my reusable block — was that left out of scope of this PR in favor of [RNMobile] Add Reusable blocks to the inserter menu #25383? I reread the PR description but I'm not clear on this.

Yes that part is not covered in this PR, it will be done in #25383.
Once this block type is registered, it adds the previously created Reusable blocks in a post to the inserter menu but only those. That's why when you create a new post there are no Reusable blocks 😄

  1. We've talked offline about reusing code before, but it was in the context of a scrollable list of elements. Looking at packages/block-library/src/block/edit.native.js, I see there is a lot of code that is the same between the native and web versions. Would it make sense to share more code here?

I totally agree, I'll review that component and identify which parts could be reused.

Thanks @guarani for all the feedback! 🎊

@fluiddot
Copy link
Contributor Author

Gutenberg on mobile doesn't support the core blocks, so it's possible to have a reusable block that's composed of some "unsupported blocks". While testing this scenario, I could see that the "Unsupported" placeholder block shows up correctly 👍 — but I'm not sure if the (?) button should be hidden. Here's a picture:

See screenshot
Wdyt @fluiddot? Maybe this needs some design input (cc @iamthomasbishop).

Oh good catch!, the problem with this component is that it disables any interaction (including the (?) icon). I'm wondering if there would be a way to enable only some parts 🤔

Another option could be to detect if there are any unsupported blocks contained in the Reusable block and present to the user something similar to the (?) icon.

@fluiddot
Copy link
Contributor Author

  1. We've talked offline about reusing code before, but it was in the context of a scrollable list of elements. Looking at packages/block-library/src/block/edit.native.js, I see there is a lot of code that is the same between the native and web versions. Would it make sense to share more code here?

I totally agree, I'll review that component and identify which parts could be reused.

I created this PR as a potential approach for reducing duplicated code. To be honest I'm not very convinced with it because it's adding maybe too many sub-components but it's a way to keep in the main component all the logic that is shared between web and mobile.

Another approach would be to add conditional rendering logic in the main file and render different components depending on the platform (web and mobile).

Finally I checked other components/blocks and I see that is more or less common to have things duplicated, do you know an example of case like this one?

@iamthomasbishop
Copy link

Maybe this needs some design input
I'm not sure if the (?) button should be hidden

@guarani I was going to respond that from a design perspective, I don't think there should be any alterations to unsupported blocks here, but just saw this comment:

the problem with this component is that it disables any interaction (including the (?) icon)

I understand not allowing interaction, that's a pretty reasonable constraint, but I would expect the nested blocks to be displayed as-is (albeit uneditable bc the parent would be "locked").

I took a moment to play around w/ a potential topper for reusable blocks, but I want to come back to it on Monday. Do y'all have any feedback? The idea is a concise header that indicates 1) locked status, 2) the block name, and 3) an info icon that when tapped would show an informational bottom sheet similar to the unsupported blocks mechanism.

I also played around with a treatment where the innerBlocks are dimmed or have an overlay to hint at inactivity, but I ended up on this after a little exploration.

@guarani
Copy link
Contributor

guarani commented Sep 18, 2020

the problem with this component is that it disables any interaction (including the (?) icon). I'm wondering if there would be a way to enable only some parts 🤔

@fluiddot sorry if I wasn't clear, I meant that since we're disabling interaction with the unsupported block (as well as any other block inside the reusable block), we might want to consider from a UX perspective to make that (?) button disappear — since the user won't be able to tap on it.
@iamthomasbishop I'm totally fine with leaving the (?) in there if you do, I thought it might be confusing to the user to have a button that can't be tapped, but could be wrong.

Finally I checked other components/blocks and I see that is more or less common to have things duplicated, do you know an example of case like this one?

I don't, sorry — it just seemed like something worth exploring. I'll check out your draft PR ASAP, but in the meantime we can leave it at that since you've looked into this and it seems like it won't be worthwhile.

@fluiddot
Copy link
Contributor Author

the problem with this component is that it disables any interaction (including the (?) icon). I'm wondering if there would be a way to enable only some parts 🤔

@fluiddot sorry if I wasn't clear, I meant that since we're disabling interaction with the unsupported block (as well as any other block inside the reusable block), we might want to consider from a UX perspective to make that (?) button disappear — since the user won't be able to tap on it.
@iamthomasbishop I'm totally fine with leaving the (?) in there if you do, I thought it might be confusing to the user to have a button that can't be tapped, but could be wrong.

@guarani no problem, I’m sorry too because I misunderstand it a bit. Ok I like the @iamthomasbishop idea about improving the header of the Reusable blocks with those elements 👍

Finally I checked other components/blocks and I see that is more or less common to have things duplicated, do you know an example of case like this one?

I don't, sorry — it just seemed like something worth exploring. I'll check out your draft PR ASAP, but in the meantime we can leave it at that since you've looked into this and it seems like it won't be worthwhile.

Ok sure, let's leave it this way. In my opinion the best option for this would be to implement the mobile version of some of the components used in that block and only create a sub-component for the wrapper (that's the only part with HTML) but maybe it's out of the scope of this PR.

@fluiddot
Copy link
Contributor Author

I took a moment to play around w/ a potential topper for reusable blocks, but I want to come back to it on Monday. Do y'all have any feedback? The idea is a concise header that indicates 1) locked status, 2) the block name, and 3) an info icon that when tapped would show an informational bottom sheet similar to the unsupported blocks mechanism.

I also played around with a treatment where the innerBlocks are dimmed or have an overlay to hint at inactivity, but I ended up on this after a little exploration.

I think that's a great idea ❤️ , if you agree I'll continue with it. The only thing that we left pending is the informational bottom sheet, I'm not sure how to deal with the texts and the design.

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

I took a moment to play around w/ a potential topper for reusable blocks, but I want to come back to it on Monday. Do y'all have any feedback? The idea is a concise header that indicates 1) locked status, 2) the block name, and 3) an info icon that when tapped would show an informational bottom sheet similar to the unsupported blocks mechanism.

I also played around with a treatment where the innerBlocks are dimmed or have an overlay to hint at inactivity, but I ended up on this after a little exploration.

I think that's a great idea ❤️ , if you agree I'll continue with it. The only thing that we left pending is the informational bottom sheet, I'm not sure how to deal with the texts and the design.

@iamthomasbishop let me know when you’re available if you agree on continuing with the changes you proposed 😉

@iamthomasbishop
Copy link

iamthomasbishop commented Sep 24, 2020

@fluiddot

if you agree I'll continue with it.

I think I'm still happy with the design 😄 Let's roll with it and iterate if necessary.

The only thing that we left pending is the informational bottom sheet, I'm not sure how to deal with the texts and the design.

Let's start with something like this:

  • Icon: Same as other informational sheet (the question-mark icon)
  • Title: Reusable blocks aren't editable on WordPress for [iOS/Android]
  • Body: no body copy
  • Actions
    • Action 1: Edit block in web browser
    • Action 2: Dismiss

Can you do me a favor and double-check that the strings on the action buttons are the same in Android to make sure we're matching the existing ones? (I'm running WPiOS at the moment, don't have my Android test phone on me)

As far as I know, the user should be able to edit this block using the UBE (unsupported block editor), so I think we're good on that front as well. // cc @etoledom can you confirm ☝️ would you expect any issues with editing reusable blocks in the UBE? Or would we handle them the same as any other unsupported block?

One other thing: it looks like in your screenshots above, we're adding some extra padding inside the block boundary. Can we remove that extra padding so the inner container aligns with other blocks on the 16px keylines? Example (red line == 16px keylines):

Thank you! I'm excited to see this moving forward, nice work thus far!

@guarani guarani self-requested a review September 24, 2020 16:39
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

When running the packager from this branch, and loading the Gutenberg editor from inside WPiOS, I don't see the site's reusable blocks in the block picker and I see this error in Metro console:

View logs BUNDLE [ios, dev] ./index.js ▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓ 100.0% (1/1), done.

WARN Network Error: {
"framesToPop": 1,
"code": "404",
"message": "Error Domain=NSURLErrorDomain Code=404 "The operation couldn’t be completed. (WordPressKit.WordPressComRestApiError error 7.)" UserInfo={NSLocalizedDescription=The operation couldn’t be completed. (WordPressKit.WordPressComRestApiError error 7.)}",
"domain": "NSURLErrorDomain",
"userInfo": {
"NSLocalizedDescription": "The operation couldn’t be completed. (WordPressKit.WordPressComRestApiError error 7.)"
},
"nativeStackIOS": [
"0 WordPress 0x0000000103a4d5c8 RCTJSErrorFromCodeMessageAndNSError + 172",
"1 WordPress 0x00000001039d4e28 41-[RCTModuleMethod processMethodSignature]block_invoke_2.129 + 168",
"2 WordPress 0x0000000103689188 $sSo8NSStringCSgACSo7NSErrorCSgIeyByyy_SSSgAGs5Error_pSgIegggg_TR + 376",
"3 WordPress 0x0000000103688d58 $s9Gutenberg013RNReactNativeA6BridgeC12fetchRequest_8resolver8rejecterySS_yypSgcySSSg_AHs5Error_pSgtctFys6ResultOyypSo7NSErrorCGcfU
+ 620",
"4 WordPress 0x000000010260f488 $s9WordPress23GutenbergNetworkRequestV06dotComE033_C5237879BCC4451BCD9D90EA1DA24FAALL4with10completionySo8NSNumberC_ys6ResultOyypSo7NSErrorCGctFyAM_So17NSHTTPURLResponseCSgtcfU0
+ 184",
"5 WordPress 0x0000000103ec7df8 $s12WordPressKit0aB10ComRestApiC7request33_936BCA6C90E859009A89D18264234057LL6method9urlString10parameters8encoding7success7failureSo10NSProgressCSg9Alamofire10HTTPMethodO_SSSDySSyXlGSgAO17ParameterEncoding_pyyXl_So17NSHTTPURLResponseCSgtcySo7NSErrorC_AWtctFyAO12DataResponseVyypGcfU0
+ 1028",
"6 WordPress 0x00000001031d04c0 $s9Alamofire11DataRequestC8response5queue0D10Serializer17completionHandlerACXDSo012OS_dispatch_E0CSg_xyAA0B8ResponseVy16SerializedObjectQzGctAA0bkF8ProtocolRzlFyycfU_yycfU_ + 360",
"7 WordPress 0x0000000102236d78 $sIeg_IeyB_TR + 52",
"8 libdispatch.dylib 0x000000010a6f7b68 _dispatch_call_block_and_release + 32",
"9 libdispatch.dylib 0x000000010a6f95f0 _dispatch_client_callout + 20",
"10 libdispatch.dylib 0x000000010a708890 _dispatch_main_queue_callback_4CF + 1000",
"11 CoreFoundation 0x00000001a1c8f1e4 472C9193-115D-34CD-AD1D-0E7E091C9432 + 647652",
"12 CoreFoundation 0x00000001a1c893b4 472C9193-115D-34CD-AD1D-0E7E091C9432 + 623540",
"13 CoreFoundation 0x00000001a1c884bc CFRunLoopRunSpecific + 600",
"14 GraphicsServices 0x00000001b870d820 GSEventRunModal + 164",
"15 UIKitCore 0x00000001a462c734 47154C6D-47DF-3ABB-A152-56B159B014E4 + 12048180",
"16 UIKitCore 0x00000001a4631e10 UIApplicationMain + 168",
"17 WordPress 0x0000000102dd62b8 main + 480",
"18 libdyld.dylib 0x00000001a194fe60 90A4E82E-250C-35E3-8B2D-51D6D8B1119B + 3680"
]
}

I can still see reusable block in the editor and in the block picker, so I'll keep testing 👍 . In any case, the above log could be worth looking into.

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

When running the packager from this branch, and loading the Gutenberg editor from inside WPiOS, I don't see the site's reusable blocks in the block picker and I see this error in Metro console:

View logs
I can still see reusable block in the editor and in the block picker, so I'll keep testing 👍 . In any case, the above log could be worth looking into.

I'm testing master branch running packager and I'm getting this error instead:

YellowBox.js:71 Possible Unhandled Promise Rejection (id: 0):
"Unsupported path: /wp/v2/posts/1?context=edit&_locale=user"

I think you're getting a 404 error because I added posts to SUPPORTED_ENDPOINTS here. Actually I'm wondering if that endpoint is really required for fetching Reusable blocks 🤔 .

@fluiddot
Copy link
Contributor Author

@iamthomasbishop Where could I access the prototype for the Reusable block title design?
Thanks!

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.
Additionally the changes have been removed from the react-native-editor CHANGELOG.
The reusable blocks are fetched in a different way that no longer require to fetch types endpoint.
Handle different site cases regarding UBE support like self-hosted with/without Jetpack.
Unbounded queries doesn't work on native version so we have to explicitly set the per_page query param with the max number of items.
@fluiddot fluiddot changed the title [RNMobile] Add support Reusable blocks [RNMobile] Reusable blocks - Present block content (preview mode) Jan 19, 2021
@fluiddot
Copy link
Contributor Author

Everything is looking good, just added a small comment I missed before 🚢

Thanks @ceyhun for the review 🎊 !
I pushed the small change regarding your comment, it was just add a comment in the code, so the PR continues ready to merge 🟢 .

I don't have permissions in the repository yet to merge so please if you think it's ready and after the checks finish, could you help me with that?
Thanks!

@ceyhun ceyhun requested a review from cameronvoell January 20, 2021 11:19
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) Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants