Page template previews #19106

koke · 2019-12-12T16:47:18Z

Description

This implements a first version of the template previews defined in wordpress-mobile/gutenberg-mobile#1466

After much wrestling with BottomSheet and react-native-modal, I couldn't find a good way to have a bottom sheet with a FlatList inside that scrolled properly. So, for now, I switched to react-native's standard Modal.

Once facebook/react-native#27618 lands on a release, we will be able to use the bottom-sheet style modals for iOS, but a cross platform solution will require more effort that's in scope for this feature.

This PR also includes some changes needed to BlockEditorProvider although I extracted those into #19120 for easier review.

This also introduces a "new" ModalHeaderBar component to show as a top bar for modals. The code came from an existing title bar in the BottomSheet code, but I haven't found any current examples of that being used in a bottom sheet. @iamthomasbishop this will need adjusting for dark mode, not sure what the colors should be there for the bar, title and close button.

@geriux I've also moved the template picker to the bottom on this PR because I needed it out of the block list. I was getting into trouble because the preview also has a block list and a BlockFooterSlot. I think I prefer your approach in #19307, and that one should replace this once merged.

So this is blocked on those three changes (BlockEditorPreview, dark mode, and moving the toolbar), but I'm about to take a week off and the bulk of the PR is ready for review

gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#1676

How has this been tested?

I tested this by forcing a postType=page and empty content in gutenberg-mobile src/index.js

Screenshots

🤔 The modal animation looks odd to me on Android, but I'm not sure what's going on there and if that's normal.

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. .

- BlockEditorProvider needs an update so it uses the subRegistry - We need a better way to only render the picker on the main block list - We need to make the bottom sheet full height, and adapt the block preview accordingly

…ration

geriux · 2019-12-31T10:04:18Z

🤔 The modal animation looks odd to me on Android, but I'm not sure what's going on there and if that's normal.

The issue here is performance, the animation of the Modal is being affected by the blocks rendering. Do we use skeleton placeholders? I think this feature would benefit from it especially on slower/low-end devices. What do you think?

geriux · 2019-12-31T10:10:07Z

@geriux I've also moved the template picker to the bottom on this PR because I needed it out of the block list. I was getting into trouble because the preview also has a block list and a BlockFooterSlot. I think I prefer your approach in #19307, and that one should replace this once merged.

Sounds good!

iamthomasbishop · 2020-01-02T21:23:17Z

Nice to see this, @koke!

The modal animation looks odd to me on Android, but I'm not sure what's going on there and if that's normal.

That does look a bit odd. At first glance, I was going to ask if we could use the same screen transition we use on other full-screen dialogs on Android that we use elsewhere (the subtle scale as the window fades in), but it sounds like this could be at least partially due to a performance issue that Gerardo mentioned.

This also introduces a "new" ModalHeaderBar component to show as a top bar for modals.

Where is this style coming from? I can replicate this in Figma and start using it where necessary (and define usage guidelines in the design docs).

Will this header style remain in place when we start using the native style of the modal sheet on iOS, or will we inherit another style header (similar to the native NavigationBar style)?

I really would love to go to that new modal bottom sheet sooner rather than later so we can take advantage of the swipe-to-dismiss functionality of the native sheets. Android is less critical to have that considering we always have an OS-level back gesture.

One other question, out of curiosity: is it possible to substitute the X for a Close text label button on iOS to mimic the native NavigationBar?

this will need adjusting for dark mode, not sure what the colors should be there for the bar, title label, and close button.

I believe the icon and header title are both white in dark mode, but can we reference WPiOS to double-check? The background of the header should also match that of WPiOS, if possible – although I think we're using a dynamic system color so we might have to mimic the default color.

The issue here is performance, the animation of the Modal is being affected by the blocks rendering. Do we use skeleton placeholders? I think this feature would benefit from it especially on slower/low-end devices. What do you think?

@geriux I think skeleton placeholders would be nice to see universally for an initial "loading" state, and I proposed something similar recently.

geriux · 2020-01-03T07:56:34Z

@geriux I think skeleton placeholders would be nice to see universally for an initial "loading" state, and I proposed something similar recently.

Nice! Yes, that would be perfect for this case 😃

koke · 2020-01-07T10:07:47Z

Happy to hear it's "just" a performance issue. I can't wait for Suspense to handle stuff like this more easily.

I generally like skeleton placeholders, but there's something odd about them for a preview like this one. Even if we are using actual blocks to render, it feels more like we are loading the preview of a web page, and I think I would expect everything to load at once. Also, for devices that are performant enough I wouldn't want to show a placeholder while the modal animation is happening if the real content can be rendered fast enough, and I'm not sure how to tell the difference without going too deep into react native.

koke · 2020-01-07T10:47:29Z

Also, for reference, this is the performance when creating a new post on the device I was testing:

geriux · 2020-01-07T14:33:29Z

I've just merged #19307 sorry for the conflicts 😅

iamthomasbishop · 2020-01-07T23:57:17Z

Also, for reference, this is the performance when creating a new post on the device I was testing:

@koke that is slow 😬 I do think skeletons would make it feel less cumbersome, but it won't completely mask the slowness.

geriux · 2020-01-08T07:57:39Z

Also, for reference, this is the performance when creating a new post on the device I was testing:

That's in prod mode, right? What specs does that device have? (I know it's not actually related to the PR but now I'm curious 😅)

koke · 2020-01-08T08:14:21Z

That's a Moto G5 Plus 😁

koke · 2020-01-08T10:40:44Z

Updated after the changes in #19307. @geriux ready for another look

pinarol · 2020-01-09T17:46:55Z

Sorry for being late to test this one. I gave it a try today with WPiOS, WPAndroid. I have no blocker findings except for the dark mode and it is a known issue.

One thing that needs tackling is, the modal is not adjusting to landscape on iOS, it stays in Portrait mode even if I rotate the device. (looks configurable)

And on Android rotating the device can cause the editor close altogether sometimes, not every time though, and this can be considered as a edge case:

Open page on portrait, tap on title
Open template modal
Rotate to landscape
At this point the template doesn't show up anymore
Rotate again
The editor closes itself

the difference of bouncing during scrolling, on iOS is it bouncing but on Android it is not. Not sure if we can control this since we use a library to display modals.

iPhoneXS, iOS 13

Huawei P20 lite, Android 9

Another thing is we have an overlap on Landscape mode but I think this is normal since we stick the template pickers on keyboard.

I also gave it a try on iPad simulator and things seem normal:

Code-wise, it looks ok to me. I'd just hope we wouldn't need to implement ModalHeaderBar but 🤷‍♂️Great job 🎉

iamthomasbishop · 2020-01-09T19:34:28Z

@koke I'm not sure if this is the right ticket to drop design feedback on, but I've got some feedback on the header specifically (some of which I mentioned last week) 😄 :

On Android (because we're using what looks like 20px font-size?) it looks like the icon isn't centered. The close icon-button should be vertically centered with the text label.
On Android, I think if we're mimicking the system App Bar style, the label should be set in Medium font-weight.
On iOS, it'd be great if we can use a text button that says Close or similar.
Are we going to add an Apply or Use button to the right side of the header?
Is it possible to also add a sub-title on the header? If so we might want to bring the title font-size down to ~16px.

Here are some exploratory mocks w/ different header variations:

geriux · 2020-01-10T08:24:45Z

Are we going to add an Apply or Use button to the right side of the header?

Yup, I have this draft PR that I will have ready for review today (don't mind the style changes you mentioned above in the draft PR because they're probably going to be updated in this one or in a new one =)

geriux

LGTM! Great job @koke ! 🎉 Approving since the styles improvements will be worked on in another PR.

Tested it on both iOS and Android ✅

geriux · 2020-01-15T09:02:03Z

packages/block-editor/src/components/page-template-picker/default-templates.js

 ];

-export default defaultTemplates;
+const parsedTemplates = memoize( () => defaultTemplates.map( ( template ) => ( {


This is super nice! 👌🏻

geriux · 2020-01-15T09:09:18Z

packages/block-editor/src/components/provider/index.native.js

-const withRegistry = createHigherOrderComponent(
-	( OriginalComponent ) => ( props ) => (
-		<RegistryConsumer>
-			{ ( registry ) => (
-				<OriginalComponent
-					{ ...props }
-					registry={ registry }
-				/>
-			) }
-		</RegistryConsumer>
-	),
-	'withRegistry'
-);


Nice clean up in favor of withRegistryProvider 👏

koke · 2020-01-15T09:34:16Z

Created wordpress-mobile/gutenberg-mobile#1777 with the style feedback to work on a separate PR

First scaffold for template previews (mobile only)

196c5bc

koke mentioned this pull request Dec 12, 2019

Add/spt template preview wordpress-mobile/gutenberg-mobile#1676

Merged

1 task

koke added 8 commits December 13, 2019 11:52

WIP: managed to make the preview show, saving as a checkpoint

6c5579b

- BlockEditorProvider needs an update so it uses the subRegistry - We need a better way to only render the picker on the main block list - We need to make the bottom sheet full height, and adapt the block preview accordingly

Set a fixed height for the template preview

b216ad5

Move template picker to the toolbar

5c744af

Read only block list

d30b426

Lint fixes

1c8531d

Made scrolling sort of working with read only block list

93379d1

A longer template to test scrolling

9fa1128

Replace BottomSheet with Modal for previews

4f0ba05

koke mentioned this pull request Dec 26, 2019

[RNMobile] Page Template Picker UI Improvements #19307

Merged

6 tasks

koke added 7 commits December 27, 2019 10:59

Merge branch 'master' into add/spt-template-preview

02a1a13

Allow closing previews with back button on Android

aad8e3f

Revert changes to BlockList that were required for bottom sheet integ…

7c9fedd

…ration

Revert changes to BottomSheet

e68c7a9

Add usage example for ModalHeaderBar

8663c04

Improve accessibility of template preview

4e86917

Improve accessibility of ModalHeaderBar

833b049

koke added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Dec 27, 2019

koke requested review from pinarol and geriux December 27, 2019 12:28

koke marked this pull request as ready for review December 27, 2019 12:30

koke requested review from ajitbohra, chrisvanpatten, ellatrix, jorgefilipecosta, talldan and youknowriad as code owners December 27, 2019 12:30

koke added 2 commits December 27, 2019 13:31

Remove unused imports

247ad17

Added missing web file

37b372f

Merge branch 'master' into add/spt-template-preview

f305785

koke mentioned this pull request Jan 9, 2020

[RNMobile] Final Page Templates with localization #19525

Merged

6 tasks

geriux approved these changes Jan 15, 2020

View reviewed changes

koke mentioned this pull request Jan 15, 2020

Style improvements for template previews wordpress-mobile/gutenberg-mobile#1777

Closed

7 tasks

koke merged commit a7db562 into master Jan 15, 2020

koke mentioned this pull request Jan 15, 2020

Consolidate web/native BlockEditorProvider #19120

Closed

6 tasks

ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020

aristath deleted the add/spt-template-preview branch November 10, 2020 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Page template previews #19106

Page template previews #19106

koke commented Dec 12, 2019 •

edited

Loading

geriux commented Dec 31, 2019

geriux commented Dec 31, 2019

iamthomasbishop commented Jan 2, 2020

geriux commented Jan 3, 2020

koke commented Jan 7, 2020

koke commented Jan 7, 2020

geriux commented Jan 7, 2020

iamthomasbishop commented Jan 7, 2020

geriux commented Jan 8, 2020 •

edited

Loading

koke commented Jan 8, 2020

koke commented Jan 8, 2020

pinarol commented Jan 9, 2020 •

edited

Loading

iamthomasbishop commented Jan 9, 2020

geriux commented Jan 10, 2020

geriux left a comment

geriux Jan 15, 2020

geriux Jan 15, 2020

koke commented Jan 15, 2020

Page template previews #19106

Page template previews #19106

Conversation

koke commented Dec 12, 2019 • edited Loading

Description

How has this been tested?

Screenshots

Checklist:

geriux commented Dec 31, 2019

geriux commented Dec 31, 2019

iamthomasbishop commented Jan 2, 2020

geriux commented Jan 3, 2020

koke commented Jan 7, 2020

koke commented Jan 7, 2020

geriux commented Jan 7, 2020

iamthomasbishop commented Jan 7, 2020

geriux commented Jan 8, 2020 • edited Loading

koke commented Jan 8, 2020

koke commented Jan 8, 2020

pinarol commented Jan 9, 2020 • edited Loading

iamthomasbishop commented Jan 9, 2020

geriux commented Jan 10, 2020

geriux left a comment

Choose a reason for hiding this comment

geriux Jan 15, 2020

Choose a reason for hiding this comment

geriux Jan 15, 2020

Choose a reason for hiding this comment

koke commented Jan 15, 2020

koke commented Dec 12, 2019 •

edited

Loading

geriux commented Jan 8, 2020 •

edited

Loading

pinarol commented Jan 9, 2020 •

edited

Loading