Refactor heading block to share more code with web version. #20745
Conversation
SergioEstevao
added
the
Mobile App - i.e. Android or iOS
SergioEstevao
requested review from
ajitbohra,
chrisvanpatten,
jorgefilipecosta,
Soean,
talldan and
youknowriad
as code owners
|
Size Change: +216 B (0%) Total Size: 860 kB
ℹ️ View Unchanged
|
| @@ -54,17 +54,19 @@ function HeadingEdit( { | |||
| onChange={ ( newLevel ) => | |||
| setAttributes( { level: newLevel } ) | |||
| } | |||
| isCollapsed={ Platform.OS === 'web' } | |||
There was a problem hiding this comment.
Do mobile toolbars support isCollapsed prop? Wht happens if we keep it true for mobile too?
There was a problem hiding this comment.
They don't show., it looks collapsed is equivalent to hidden in mobile.
There was a problem hiding this comment.
Do you think we should update the toolbar implementation to just ignore that prop instead?
There was a problem hiding this comment.
I can update the code for the native ToolbarGroupCollapsed to simple do the same as the non-collapsed version, until we get a proper implementation of a collapsed toolbar for mobile.
| @@ -81,7 +83,9 @@ function HeadingEdit( { | |||
| <RichText | |||
| ref={ ref } | |||
| identifier="content" | |||
| tagName={ Block[ tagName ] } | |||
| tagName={ | |||
| Platform.OS === 'web' ? Block[ tagName ] : tagName | |||
There was a problem hiding this comment.
I think Block component should have an equivalent in native. This is going to become a central component to write blocks. Ideally all new blocks use it. cc @ellatrix
There was a problem hiding this comment.
Yes, ideally native has a Block component too. I'm not sure what it will take there. I don't know enough about the native side, but happy to collaborate with someone.
There was a problem hiding this comment.
I'm working on to have an equivalent ( more a mock at this stage) on this PR: #20772
Will update this PR when the above is merged in.
@SergioEstevao I know the web extracts the heading levels in their entirety into the inspector, but I’ve always felt this was odd, especially when ported as-is to mobile. I think these should all be inside the block toolbar, and while I understand there is reasonable rationale for not surfacing H1s, I don’t love the idea of losing H5 and H6 in the toolbar. Maybe this is a good opportunity to consider tackling collapsible groups for things like this (also for alignment controls, etc). I remember having some earlier discussions on this issue, but can’t seem to find that discussion atm. I’ll try to find it tomorrow and lay out a few rough sketches as well to demonstrate what the potential paths are. |
@iamthomasbishop One advantage of not having those is that it makes the alignment buttons visible on the toolbar, without need to scroll. I think we have a global problem in the toolbar when we have more commands that are not in the screen, I don't think our users know that they can scroll horizontally on the toolbar to get more commands. |
We now have an implementation of Block for RN.
Both good points, although I think the scrolling concern is a more complex discussion that we should have separately. For now, implementing collapsible groups would mitigate the bulk of the cases where there is overflow so we should probably start there. |
I think this will be a broader work, and should be in a separate PR of this one. Meanwhile you prefer to have all header level in the toolbar and the block settings removed for mobile? |
@SergioEstevao Agree if we do further exploration, but before we push that work aside, let me propose what could be a nice temporary solution 😄 If it will require much additional effort, let me know and we will proceed with the solution mentioned below. What do you think about doing something extremely simple like tap-to-cycle-headings in the toolbar? The heading buttons would be collapsed into a single button, and tapping it would cycle through heading levels. In theory, we could do the same thing with alignment, etc. Here's a quick and dirty prototype for example:
If you think the above would be more effort, for now, let's consider doing one of the following (in no particular order):
Let me know if you have any preferences or questions @SergioEstevao. |
@iamthomasbishop at the moment I have the above implemented, so it's exactly like the web. I can try to give it a go on the rotating buttons idea, but I have a feeling that it will very confusing for our users... |
As a temporary measure (temp. because we will be establishing a more holistic solution at some point), what do you think about relying on a native component like the
I think this would be pretty intuitive for a user and allow us to apply the pattern to other groups, such as alignment, etc. in a way that doesn't seem very expensive. |
|
I gave it a go and implement the dropdown like this:
What do you say @iamthomasbishop ? |
We also hide the headings settings in RN because all of them are available in the block controls.
|
@iamthomasbishop I followed your feedback and here is the latest version:
I styled the dropdown menu like the block settings and I added the checkmark icon to show current option selected. |
| renderContent={ ( { isOpen, onClose, ...props } ) => { | ||
| return ( | ||
| <BottomSheet | ||
| hideHeader={ true } | ||
| isVisible={ isOpen } | ||
| onClose={ onClose } | ||
| > | ||
| { isFunction( children ) ? children( props ) : null } | ||
| <PanelBody title={ label }> |
There was a problem hiding this comment.
Could we use the Picker component to reuse functionality? (packages/components/src/mobile/picker/)
There was a problem hiding this comment.
The Picker is using an ActionSheet in iOS that doesn't translate to the interface we want here.
Or are you saying to refactor the Picker component to be like like this visually?
There was a problem hiding this comment.
Oh, I thought we wanted the ActionSheet on iOS based on @iamthomasbishop 's comment.
Still, as this layout is already in place somewhere else, maybe we could add a prop to the picker component, something like useNativeActionSheet, and use the code currently from the Android implementation if that prop is false. Anyway, not sure if this is the best way to go or if it will make things unnecessary complicated.
|
@SergioEstevao I think this is looking pretty solid, and if we do stay this route (BottomSheet), I have some suggestions in terms of design details to shore up. Per @etoledom's question re: actionSheets, I'm not opposed to staying with a more native component like ActionSheet on iOS, but I would assume we're going to need a BottomSheet for Android. Feedback on BottomSheet as you've shown above:
|
|
@iamthomasbishop I addressed all the issues you commented above, here is the end result: Android
iOS light mode
iOS dark mode
|
There was a problem hiding this comment.
Nice work @SergioEstevao ! I had a couple thoughts, but otherwise I believe it's good to go 👍
|
@SergioEstevao Looking good, just a couple minor requests to wrap up, and I've provided a visual example below:
Also likely not related to this PR, but if possible on Dark Mode, can we map the heading icons on the sheet and in the toolbar to the neutral gray? // cc @etoledom because I think you did some work on this recently, maybe you have a suggestion. |
|
@iamthomasbishop here is a screenshot with the updated iOS and Android versions:
|
|
Looks solid! Ship it! 🚀 |
| @@ -97,6 +99,7 @@ function HeadingEdit( { attributes, setAttributes, mergeBlocks, onReplace } ) { | |||
| [ `has-text-align-${ align }` ]: align, | |||
| } ) } | |||
| placeholder={ placeholder || __( 'Write heading…' ) } | |||
| textAlign={ align } | |||
There was a problem hiding this comment.
This is the only thing that's bothering me, in this PR. It's redundunt with the style prop in the web and it can easily get removed since it's not used at all in the web version.
There was a problem hiding this comment.
This is a recurrent issue we have on the mobile implementations. We don't have access to CSS so we end up recurring to extra props or container classes to do the work that CSS does. I'm all open to suggestions on how can we avoid these issues.
@mtias told me there is some approach being discussed about Global Styling approach using props for the components that could be a way to make the implementation less dependent of CSS styling.
Description
This PR refactors the heading block to share the edit function with the web version. On the process, it adds the align button to the header block in mobile.
It also reduces the number of headers available on the block toolbar in order to make the align buttons visible without the need for scrolling.The only issue that I have is the way it shows the extra headers level on the Block setting modal@iamthomasbishop Should we convert this to another type of setting, that looks better on mobile?We now have an implementation of the dropdown menu in mobile that uses a bottom sheet to show the options available.
How has this been tested?
This can be tested on GB-mobile using this PR: wordpress-mobile/gutenberg-mobile#1990
Screenshots
Types of changes
Checklist: