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

Experimental page template selector #18052

Merged
merged 23 commits into from
Dec 2, 2019
Merged

Conversation

koke
Copy link
Contributor

@koke koke commented Oct 21, 2019

Description

Introduces the foundations for Starter Page Templates (#18055). This PR introduces several pieces:

  • A new Page Templates experiment to use as a feature flag for this feature while in development
  • A BlockListFooter slot-fill to be able to conditionally insert the template picker at the end of the block list. This might change in the future as the native UI will likely be attached to the toolbar, and there's not enough clarity yet on what will happen on the web. For now, it offers a good starting point.
  • A withPageTemplatePickerVisible HOC that encapsulates the logic on when the template picker should be visible (only on new/empty pages, if the experiment is enabled)
  • A PageTemplatePicker component that shows the available templates (we hardcoded 2 for now) and replaces the post content when one is selected.

Fixes wordpress-mobile/gutenberg-mobile#1464 and partially addresses wordpress-mobile/gutenberg-mobile#1463 (the feature flag exists, but we'll need more changes in the bridge and initialization to make it work on mobile).

How has this been tested?

On mobile:

On web:

  • Go to Gutenberg > Experiments and enable the Page Templates experiment
  • Create a new page. The template picker should be visible
  • Pressing a template should replace the page contents and make the picker disappear
  • Pressing undo should remove the content and bring back the picker
  • Creating a new post should not show the template picker
  • Opening a new post or page should not show the template picker

Screenshots

spt-ios

Screen Shot 2019-11-13 at 18 24 02
Screen Shot 2019-11-13 at 18 24 24
Screen Shot 2019-11-13 at 18 24 26
Screen Shot 2019-11-13 at 18 24 31

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.

It's very rough, but mostly focused on where is the right place to put the new
components and how to connect them.
@koke koke added the [Status] In Progress Tracking issues with work in progress label Oct 21, 2019
@koke koke changed the title Add experimental page template selector Experimental page template selector Oct 21, 2019
Exporting Fragment directly would fail, since Fragment won't take the style prop
They don't make sense right now as a standalone component, and will likely move to the store in the future
@koke koke requested review from pinarol and mkevins November 13, 2019 17:34
@koke koke added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) and removed [Status] In Progress Tracking issues with work in progress labels Nov 13, 2019
@koke koke marked this pull request as ready for review November 13, 2019 17:34
Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

This is looking pretty good to me, I tested on both web and mobile. I am just dropping a discussion comment about visibility mechanism of buttons.

import { useSelect } from '@wordpress/data';

const __experimentalUsePageTemplatePickerVisible = () => {
return useSelect( ( select ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL: didn't know we had useSelect

const __experimentalPageTemplatePicker = ( { templates = defaultTemplates, resetContent } ) => {
return (
<__experimentalBlockListFooter>
<Container style={ { flexDirection: 'row' } }>
Copy link
Contributor

Choose a reason for hiding this comment

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

Another possibility I considered is to remove the Container here, and wrap the slot in block-list.native.js instead, so we can avoid the platform divergence here.

For example, this seems to work as well
diff --git a/packages/block-editor/src/components/block-list/index.native.js b/packages/block-editor/src/components/block-list/index.native.js
index 9230407..a453721 100644
--- a/packages/block-editor/src/components/block-list/index.native.js
+++ b/packages/block-editor/src/components/block-list/index.native.js
@@ -164,7 +164,9 @@ export class BlockList extends Component {
 				} } >
 					<View style={ styles.blockListFooter } />
 				</TouchableWithoutFeedback>
-				<__experimentalBlockListFooter.Slot />
+				<View style={ { flexDirection: 'row' } }>
+					<__experimentalBlockListFooter.Slot />
+				</View>
 			</>
 		);
 	}
diff --git a/packages/block-editor/src/components/page-template-picker/container.js b/packages/block-editor/src/components/page-template-picker/container.js
deleted file mode 100644
index de666e9..0000000
--- a/packages/block-editor/src/components/page-template-picker/container.js
+++ /dev/null
@@ -1,9 +0,0 @@
-const Container = ( { children } ) => {
-	return (
-		<>
-			{ children }
-		</>
-	);
-};
-
-export default Container;
diff --git a/packages/block-editor/src/components/page-template-picker/container.native.js b/packages/block-editor/src/components/page-template-picker/container.native.js
deleted file mode 100644
index d987d01..0000000
--- a/packages/block-editor/src/components/page-template-picker/container.native.js
+++ /dev/null
@@ -1,6 +0,0 @@
-/**
- * External dependencies
- */
-import { View } from 'react-native';
-
-export default View;
diff --git a/packages/block-editor/src/components/page-template-picker/picker.js b/packages/block-editor/src/components/page-template-picker/picker.js
index 886c649..68d71d3 100644
--- a/packages/block-editor/src/components/page-template-picker/picker.js
+++ b/packages/block-editor/src/components/page-template-picker/picker.js
@@ -9,21 +9,18 @@ import { withDispatch } from '@wordpress/data';
  */
 import __experimentalBlockListFooter from '../block-list-footer';
 import Button from './button';
-import Container from './container';
 import defaultTemplates from './default-templates';
 
 const __experimentalPageTemplatePicker = ( { templates = defaultTemplates, resetContent } ) => {
 	return (
 		<__experimentalBlockListFooter>
-			<Container style={ { flexDirection: 'row' } }>
-				{ templates.map( ( { name, content } ) => (
-					<Button
-						key={ name }
-						onPress={ () => resetContent( content ) }
-						label={ name }
-					/>
-				) ) }
-			</Container>
+			{ templates.map( ( { name, content } ) => (
+				<Button
+					key={ name }
+					onPress={ () => resetContent( content ) }
+					label={ name }
+				/>
+			) ) }
 		</__experimentalBlockListFooter>
 	);
 };

But I wasn't sure if there were other future reasons for the container here. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the horizontal layout is specific to the picker, which kind of looks like a toolbar. The block footer is an agnostic container and I would expect that if I added several components there they would stack vertically by default.

I don't think there's a platform divergence here in terms of UX, it just happens that on the web we get the layout we want for free because Button seems to default to display: inline-flex and they stack horizontally. We don't have a clear vision of what the UX would be for this on the web, but for now the container approach seems to me like the more semantically correct one.

Copy link
Contributor

@mkevins mkevins Nov 29, 2019

Choose a reason for hiding this comment

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

By "platform divergence", I just meant the two container{.native,}.js files, and that for web it's just a fragment (props silently ignored), and mobile it's a View, and can accept all View props.

The possibility of additional components in the BlockListFooter, and the expectation it would have flex-direction: column; makes sense to me. Thanks for the explanation! I suppose I'd consider that a future reason :) (and I agree the UX is not clear yet). 👍

@koke koke requested review from pinarol and mkevins November 29, 2019 11:34
@koke
Copy link
Contributor Author

koke commented Nov 29, 2019

Solved conflicts (again, twice 😩) and pushed a change to also show the picker if the user types and then deletes the content (without using undo). I re-requested a review, but then I noticed it was approved already, so I'll start merging once green (this PR seems too prone to conflicts) and we can address any new feedback on another PR

Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

I like the idea of isUnmodifiedDefaultBlock even better. Approving again.

@koke koke merged commit 24c207a into master Dec 2, 2019
@koke koke deleted the add/experimental-page-templates branch December 2, 2019 09:52
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Basic template picker
4 participants