-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
It's very rough, but mostly focused on where is the right place to put the new components and how to connect them.
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
There was a problem hiding this 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.
packages/block-editor/src/components/page-template-picker/use-page-template-picker-visible.js
Outdated
Show resolved
Hide resolved
import { useSelect } from '@wordpress/data'; | ||
|
||
const __experimentalUsePageTemplatePickerVisible = () => { | ||
return useSelect( ( select ) => { |
There was a problem hiding this comment.
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' } }> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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). 👍
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 |
There was a problem hiding this 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.
Description
Introduces the foundations for Starter Page Templates (#18055). This PR introduces several pieces:
Page Templates
experiment to use as a feature flag for this feature while in developmentBlockListFooter
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.withPageTemplatePickerVisible
HOC that encapsulates the logic on when the template picker should be visible (only on new/empty pages, if the experiment is enabled)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:
Page Templates
experimentScreenshots
Types of changes
New feature
Checklist: