Extract separator component out of nextpage block #14175

koke · 2019-02-28T18:27:41Z

This is a proof of concept, do not merge. Also note that this is targeting the gutenberg-mobile develop branch

This PR extracts the horizontal separator into a separate component, effectively making the nextpage block cross-platform (running the same code for web and mobile).

I think Separator would need more work and a better name before merging: we have a similar thing in the more and separator blocks, so we would have to make sure the component covers all the right cases, and it's customizable enough.

However I wanted to put up this PR as an example so we can start discussing if this looks like the right approach.

gziolo

The component itself might make sense but I would prefer to double check whether changing the Next Page block is expected.

/cc @jasmussen @kjellr

gziolo · 2019-03-04T17:11:32Z

packages/block-library/src/nextpage/edit.js

-		<div className="wp-block-nextpage">
-			<span>{ __( 'Page break' ) }</span>
-		</div>
+		<Separator customText={ __( 'Page break' ) } />


At this moment it might be considered as a breaking change to stop using wp-block-nextpage class in here. I'm sure that themes already apply their customizations based on this class.

Would it be fine to pass a className prop to Separator and have it render it that way, or do we want to ensure there's a wrapper div around whatever the implementation for separator is?

jasmussen · 2019-03-05T08:31:26Z

Thanks for the ping @gziolo, can you clarify what you'd like input on? Is there a visual change? I don't have my Gutenberg Mobile set up properly so it's hard for me to test this.

Are we discussing the visual separator block (maps to hr tag)? Or are we discussing the "magic" nextpage blocks, or the "read more" block that causes a jump from the blog homepage? Or all three?

The last two, read more and nextpage should visually look mostly the same, which is a dashed separator.

gziolo · 2019-03-05T08:36:01Z

Are we discussing the visual separator block (maps to hr tag)? Or are we discussing the "magic" nextpage blocks, or the "read more" block that causes a jump from the blog homepage? Or all three?

This PR was opened to explore whether we should introduce a new reusable/shared component which visually looks like nextpage. That's all :)

jasmussen · 2019-03-05T08:37:18Z

Ah, gotcha. Yeah, if people use that (I know it's an existing WordPress feature), then makes total sense. As noted, I'd use the "read more" block as a guideline for how such a block should look 👍 👍

koke · 2019-03-06T09:37:43Z

For context, this PR was done to explore how would we write a block in a cross-platform way, using the exact same code for web and native, and extracting all elements into components.

What I imagined this Separator component to be is something customizable enough to display the rendering for nextpage, more, and all the styles of separator

On the native side, we've been using react-native-hr, so a similar component could make sense.

koke · 2019-03-08T21:41:38Z

After looking into this a bit more, the separator block variants seem more complicated as they depend on block styles that can be overridden by the site's theme. We'll have to deal with that eventually, but right now I don't see how we can handle that in an isolated component that works across platforms.

I think this Separator could stay as the dashed line with (optionally editable) text in the middle, and then we can deal with making <hr> cross-platform separately

gziolo · 2019-03-18T07:36:52Z

@mapk @karmatosed, how do you feel about adding new Seperator component:
https://github.com/WordPress/gutenberg/blob/574b55f8edc54bcdcbbbd55d29e0de7896a66744/packages/components/src/separator/index.js

I'm mostly interested whether it fits to the overall picture of how we want to drive WordPress components package.

gziolo · 2019-03-18T07:50:39Z

I asked for more insights from #design team on WordPress.org Slack (link requires registration at https://make.wordpress.org/chat/):
https://wordpress.slack.com/archives/C02S78ZAL/p1552894806482400

mapk · 2019-03-19T03:59:24Z

I think this Separator could stay as the dashed line with (optionally editable) text in the middle, and then we can deal with making
cross-platform separately

This sounds right to me. Thanks for looking into this, @koke! If we can componentize things like this to allow for reusable code across platforms, that would be awesome!

@mapk @karmatosed, how do you feel about adding new Seperator component:

I'm all for it. Do we need to add this to our list for documenting? Or should we wait for the final product first?

gziolo

It looks like this component is ready to be tested and documented. Personally, I think that documentation is an important part of exposing new reusable components.

gziolo · 2019-03-19T07:58:19Z

packages/components/src/separator/index.js

+			onKeyDown={ onKeyDown }
+		/>
+	);
+	const label = ( editable ? editableLabel : staticLabel );


editable prop is redundant, you can assume that component is editable if onChange or onKeyDown prop were passed.

I'm having a hard time saying yes 😁 I think it makes sense in a way, since I can't think of a scenario where you'd want an editable field and not care about when it changes, but conceptually it seems like getting a bit too smart at changing what the component is based on these attributes.

What if the whole label (editable or not) was passed as a child component? We'd still need to create the cross-platform equivalent of TextInput and Text, but it also means that if there are no children passed, this can also be used for the HorizontalRule component we were discussing in #14361

hypest · 2019-03-27T08:47:36Z

It looks like this component is ready to be tested and documented. Personally, I think that documentation is an important part of exposing new reusable components.

👋 @gziolo , what are the details you'd like to see documented? Is the README.md of the toogle-control component a good example perhaps? What else besides a README you think would be desirable to add perhaps? Sorry if there are documentation guidelines already in place that I have missed.

gziolo · 2019-03-28T12:12:14Z

https://github.com/WordPress/gutenberg/tree/master/packages/components/src/form-toggle - FormToggle has great documentation, but ToogleControl (you shared) is also a good example as it builds on top of FormToggle.

gziolo · 2019-05-10T07:10:12Z

@koke what's blocking this PR from being finished? Is it still valid?

koke · 2019-05-10T09:24:53Z

It needs more work with testing and documentation, but I haven't found the time to focus on this one. Since this is not even targeting master and I would need to rebase the changes anyway, I'm happy to close this one if you want until I'm ready to work on it again

gziolo · 2019-05-10T10:05:20Z

It’s fine to leave it as is. I wanted to ensure it’s still actionable 👍

Extract separator component out of nextpage block

05d9297

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 Feb 28, 2019

koke requested review from aduth, ajitbohra, chrisvanpatten, gziolo, jorgefilipecosta, noisysocks, notnownikki, Soean, talldan and youknowriad as code owners February 28, 2019 18:27

gziolo added the Needs Design Feedback Needs general design feedback. label Mar 4, 2019

gziolo reviewed Mar 4, 2019

View reviewed changes

koke added 2 commits March 8, 2019 23:05

Make Separator label editable and use in more block

77d551f

Add className support to Separator§

574b55f

gziolo reviewed Mar 19, 2019

View reviewed changes

koke mentioned this pull request Mar 25, 2019

Refine "Page Break" Block designs wordpress-mobile/gutenberg-mobile#618

Open

koke mentioned this pull request Apr 4, 2019

Visual refinements for native nextpage #14826

Merged

koke mentioned this pull request Jun 6, 2019

More block enhancements wordpress-mobile/gutenberg-mobile#1079

Merged

1 task

SergioEstevao closed this Oct 2, 2019

youknowriad deleted the rnmobile/xp-nextpage branch May 27, 2020 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract separator component out of nextpage block #14175

Extract separator component out of nextpage block #14175

koke commented Feb 28, 2019

gziolo left a comment

gziolo Mar 4, 2019

koke Mar 6, 2019

jasmussen commented Mar 5, 2019

gziolo commented Mar 5, 2019

jasmussen commented Mar 5, 2019

koke commented Mar 6, 2019

koke commented Mar 8, 2019

gziolo commented Mar 18, 2019

gziolo commented Mar 18, 2019

mapk commented Mar 19, 2019

gziolo left a comment

gziolo Mar 19, 2019

koke Mar 19, 2019

hypest commented Mar 27, 2019

gziolo commented Mar 28, 2019

gziolo commented May 10, 2019

koke commented May 10, 2019

gziolo commented May 10, 2019

Extract separator component out of nextpage block #14175

Extract separator component out of nextpage block #14175

Conversation

koke commented Feb 28, 2019

gziolo left a comment

Choose a reason for hiding this comment

gziolo Mar 4, 2019

Choose a reason for hiding this comment

koke Mar 6, 2019

Choose a reason for hiding this comment

jasmussen commented Mar 5, 2019

gziolo commented Mar 5, 2019

jasmussen commented Mar 5, 2019

koke commented Mar 6, 2019

koke commented Mar 8, 2019

gziolo commented Mar 18, 2019

gziolo commented Mar 18, 2019

mapk commented Mar 19, 2019

gziolo left a comment

Choose a reason for hiding this comment

gziolo Mar 19, 2019

Choose a reason for hiding this comment

koke Mar 19, 2019

Choose a reason for hiding this comment

hypest commented Mar 27, 2019

gziolo commented Mar 28, 2019

gziolo commented May 10, 2019

koke commented May 10, 2019

gziolo commented May 10, 2019