Refactor BlockToolbar out of BlockList by Tug · Pull Request #16677 · WordPress/gutenberg

Tug · 2019-07-19T11:56:50Z

Description

With InnerBlocks coming up in #16305 we needed to move BlockToolbar out of BlockList to avoid having it created for each BlockList instance. This ended up being a large refactor as a lot of things are related to BlockToolbar.

First we need to revisit a few conventions:

BlockToolbar should correspond to the controls that we display when a block is selected (BlockControls and BlockFormatControls at the moment for native), not the whole bottom toolbar.
Those controls should be inlined into the main toolbar that controls inserting new blocks, hiding the keyboard etc. For this we can create our own native version of PREFERENCES_DEFAULTS in the edit-post store and set the fixedToolbar setting to true.
This main toolbar correspond to HeaderToolbar on the web, even though it's at the bottom on native mobile, we'll keep the name since it really matches the component features.

Thus we can see that refactoring BlockToolbar actually means refactoring the whole toolbar, meaning the block picker should be moved out of BlockList as well. Same for the hide keyboard control.
Moving the block picker out of BlockList means we need to move the "ADD BLOCK HERE" component out as well, since it depends on the state of the block picker (open or closed). And since gutenberg already supports all those features on the web we should be able to achieve this by keeping the same structure and making native version of those components.

Here is the list of changes and their reason, sorted by alpabetical order of components (following as the order in the Files changed tab):

~~BlockListAppender is a simple wrapper of DefaultBlockAppender that is designed to display a new component at the end of a list to keep the writing flow.~~ (reverted to reduce changes)
~~Block: Add a BlockInsertionPoint that displays "ADD BLOCK HERE" at the right position when the block picker is open~~ (reverted to reduce changes)
BlockList
- Remove ListEmptyComponent from KeyboardAwareFlatList as it can be achieved using ListFooterComponent only since they use the same component BlockListAppender to create a new block, only the placeholder changes ("Start writing…" when the list is empty, otherwise leave blank)
- ~~Remove renderAddBlockSeparator from renderItem as it's added in Block as the BlockInsertionPoint~~ (reverted to reduce changes)
- Remove everything related to the block type picker
- Remove everything related to showing and hiding the keyboard
- Remove BlockToolbar
- Remove renderHTML which is already unused
~~BlockInsertionPoint, now contains the "ADD BLOCK HERE" component and is connected to the store to find out if it should render or not.~~ (reverted to reduce changes)
BlockToolbar reduced functionalities from the whole editor toolbar to just the block controls and format controls
DefaultBlockAppender update to allow empty placeholder to be given and reuse the web logic to determine if it should be visible or not (visible only if the last block is not a paragraph for instance)
Inserter groups the block inserter button with the block type picker modal and reuses Dropdown for its own state (modal shown or hidden)~~ (reverted to reduce changes)
InserterMenu contains the whole block type picker modal and is responsible for updating the insertion point index in the store (so the BlockInsertionPoint can be rendered)
Dropdown a simple native version of the web Dropdown component which is responsible for showing and hidding the modal when we click on the toggle button
HeaderToolbar is now our global toolbar which contains the inserter, the block toolbar and the hide keyboard button
Header wraps HeaderToolbar
Layout is now responsible for displaying the root BlockList along with the Header
VisualEditor we remove BlockEditorProvider from it since we now use EditorProvider in Editor which does the same thing and more
Editor we move all the gutenberg bridge listeners to EditorProvider that we now call in our render function along with SlotFillProvider
EditorProvider this native version handles setting the gutenberg native bridge listeners and switching mode, it also calls the web version of EditorProvider which calls setupEditor and instantiates BlockEditorProvider. For this to work we needed to mock ReusableBlocksButtons and ConvertToGroupButtons.

How has this been tested?

Screenshots

Types of changes

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.

…ditorProvider to Editor. Plus support InsertionPoint and BlockListAppender

hypest · 2019-07-19T13:06:00Z

we needed to move BlockToolbar out of BlockList to avoid having it created for each BlockList instance

Question: is it already out of the BlockList on the web codepaths? In other words, are we aligning the mobile to the web or are we refactoring both codepaths?

Tug · 2019-07-19T13:13:41Z

@hypest Yes, we're aligning with the web with this change :)
And not just on the BlockToolbar, also on the way the editor is initialized, on the way we display the "ADD BLOCK HERE" component using the store, and many other small refactors that bring our components logic closer to their web version counterparts.

hypest · 2019-07-19T13:13:56Z

BlockListAppender is a simple wrapper of DefaultBlockAppender that is designed to display a new component at the end of a list to keep the writing flow.

Probably a naive question but, how's this related to the BlockToolbar? In other words, why do we need to introduce the BlockListAppender?

Tug · 2019-07-19T13:47:30Z

why do we need to introduce the BlockListAppender?

This is because I brought some logic from the web in DefaultBlockAppender, namely the ability to determine if we should display it or not, depending if the last block is a paragraph or if the block list is empty or not. This logic required new properties to be added to DefaultBlockAppender which would make BlockList a tiny bit more complex . My goal was to simplify it so I reused BlockListAppender which basically connects a DefaultBlockAppender to a given BlockList using the rootClientId. Granted it's not a "required" change but a nice to have

hypest · 2019-07-19T14:00:57Z

To be honest, I wanted us to start using BlockListAppender for ages, since it's functionality we wanted on the native mobile and only recently got around implementing it. That said, I'd prefer to not have it in this PR which is about the BlockToolbar refactor. Will it be hard to extract it into a followup PR? Actually, let's do the same (extract in follow up PR(s)) any of the changes that are not related to or trickle down from the BlockToolbar refactor.

I like narrow-er scope PRs 😄🤷‍♂️ , both because they are easier to review, and because of the smaller test surface they require.

Tug · 2019-07-19T14:09:28Z

Will it be hard to extract it into a followup PR? Actually, let's do the same (extract in follow up PR(s)) any of the changes that are not related to or trickle down from the BlockToolbar refactor.

👍 I already started doing that ;)

hypest · 2019-07-19T14:25:49Z

Hey @Tug , on first look, the approach here (to move even more things/componets to match the structure in the web codepath) feels solid to me and I'd like to see this PR go through!

Can you open sibling PRs on gutenberg-mobile and WPAndroid so we can be sure nothing obvious gets broken? For example, yarn test against this PR on gb-mobile seems to fail atm. Otherwise, simple run of WPAndroid with this seems to work nicely so, this already looks good on the smoke testing side.

…n keyboard is shown

Tug · 2019-07-24T15:59:57Z

Rebasing/merging this with master introduces regression in the post title selection and new block insertion. #16704 aims to fix this

…lock-toolbar

Tug · 2019-07-25T17:07:46Z

Should be almost ready for review now, I just need to do a full check up on it since the last merge might have introduced regressions in the ADD BLOCK HERE feature. I also need to test on iOS since I have not and fix the hard coded value (40px here and there) I added for styling.

Other than that, should be good for a first pass on the code :)

mchowning · 2019-07-26T12:33:07Z

Nice work @Tug! I'm not seeing any crashes or other major issues. I do see a few regressions with respect to block insertion though:

resolved - ~~Blocks inserted from the post title are no longer being inserted at the beginning (instead of the end) of the post (recent PR adding this)~~
resolved - ~~There is no ABH indicator when inserting a post from the post title (recent PR adding this)~~
When inserting a block from an empty paragraph, the ABH indicator appears after the empty paragraph instead of before/in-place-of it (PR that addressed this)

Tug · 2019-07-26T12:39:29Z

@mchowning thanks for testing and reporting! I thought I actually fixed that but it seems I did not push 😱. Sorry for the time wasted there.

I still need to look at 3. as I don't think this is addressed with my last commit

mchowning · 2019-07-26T12:51:04Z

No worries. I retested, and you're right, 1 and 2 are now resolved. Unfortunately, I did notice one more edge case that appears to have regressed, so the two outstanding issues that I see are:

Previously noted: When inserting a block from an empty paragraph, the ABH indicator appears after the empty paragraph instead of before/in-place-of it (PR that addressed this)
New: When the top block of a post is an empty paragraph block and a new block is being inserted from the post title, the empty paragraph is not replaced. (recent PR that fixed this).

FWIW, these both seem like relatively minor regressions to me, and if they're not easy fixes I wouldn't be opposed to merging with the regressions after 1.10 is branched off and just fixing the regressions in separate PRs (could even divide that work up). This is a big PR and the sooner we can get it merged the better imo.

Tug · 2019-07-31T10:08:21Z

@mchowning Yes indeed I noticed those while debugging the e2e tests failing. I'm looking for a fix atm, but it doesn't seem trivial, would appreciate a hand anyone has spare time :)

…lock-toolbar

…new text based block

This reverts commit 59f0431.

…lock-toolbar

mchowning · 2019-08-02T21:23:39Z

-		super( ...arguments );

-		this.onKeyboardHide = this.onKeyboardHide.bind( this );
+export const BlockToolbar = ( { blockClientIds, isValid, mode } ) => {


Not a big deal at all, but what would you think about combining the render-nothing cases instead of having two different ones (the first returning null and the second returning <></>? To my eyes, something like this is easier to follow:

export const BlockToolbar = ( { blockClientIds, isValid, mode } ) => { const showToolbar = blockClientIds.length !== 0 && mode === 'visual' && isValid; return showToolbar ? ( <> <BlockControls.Slot /> <BlockFormatControls.Slot /> </> ) : null; };

I don't feel strongly about this, so feel free to leave it as-is if you prefer.

Indeed it's a bit better 👍
I think we can keep the current syntax though as it's closer to the web version and we don't delay this PR much longer as well. Let's revisit later if necessary

mchowning

Tested on both platforms and everything appears to be working. Great work @Tug! 🎉 🎉

I did leave one minor code style comment, but it is only a suggestion.

Tug · 2019-08-05T11:48:19Z

@mchowning Thanks a lot for the review 🙇

pinarol · 2019-08-06T17:11:27Z

-					innerToolbarHeight={ blockMobileToolbarHeight }
-					safeAreaBottomInset={ this.props.safeAreaBottomInset }
-					parentHeight={ this.props.rootViewHeight }
+					extraScrollHeight={ innerToolbarHeight + 10 }


what does "+10" refer to?

pinarol

I entered 3 comments that need to be addressed before we go to production

pinarol · 2019-08-06T17:12:15Z

 					innerRef={ this.scrollViewInnerRef }
-					blockToolbarHeight={ toolbarHeight }
-					innerToolbarHeight={ blockMobileToolbarHeight }
-					safeAreaBottomInset={ this.props.safeAreaBottomInset }


we need this to be able to scroll properly in iPhone X variety devices

pinarol · 2019-08-06T17:13:15Z

 					accessibilityLabel="block-list"
 					innerRef={ this.scrollViewInnerRef }
-					blockToolbarHeight={ toolbarHeight }
-					innerToolbarHeight={ blockMobileToolbarHeight }


we need this to be able to show the block toolbar when we are focused on the last line of the text

etoledom · 2019-08-07T13:11:13Z

Hey @Tug !

I did a first check of automatic scrolling on iOS and it looks good overall. I did find a couple of small regressions but I don't think they are blockers for a release. It would be good to check them out anyway. I'll continue testing more in depth and create an issue with the details.

I did find a regression that I believe is a blocker and we should fix it before going to production:

The toolbar lost its bottom safe are. This is reproducible on the iPhone X family devices

Please let me know if I can help in any way!

Tug · 2019-08-07T13:59:54Z

Oh indeed, I did that on purpose because I thought that's what we had but looking at the production app I see it's not the case. That should be easily to fix 👍

* Move BlockToolbar from BlockList to Layout * Remove BlockEditorProvider from BlockList and add native version of EditorProvider to Editor. Plus support InsertionPoint and BlockListAppender * Revert BlockListAppender and InsertionPoint additions * Fix dismissing block picker * Add missing function in BlockList * Disable add block in HTML mode and show hide keyboard button only when keyboard is shown * Fix bringing back finishInsertingOrReplacingBlock * Fix inserting block in first position when post title is selected * Show insertion point before block if its replaceable * Fix missing shouldPreventAutomaticScroll props for iOS * Fix native tests * Add back bottom View to push block list up * Improve defining toolbar height * Make html view a flexbax * Quickly hide the modal to let the keyboard show up after inserting a new text based block * Let's unmount the modal instead to make sure we don't have timing errors * revert to defining the toolbar height in the component itsef * Revert "Make html view a flexbax" This reverts commit 59f0431. * Simplify layout * Fix dismiss keyboard on iOS

Tug added 2 commits July 17, 2019 14:49

Move BlockToolbar from BlockList to Layout

644e045

Remove BlockEditorProvider from BlockList and add native version of E…

0dd26db

…ditorProvider to Editor. Plus support InsertionPoint and BlockListAppender

Tug self-assigned this Jul 19, 2019

Tug added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Status] In Progress Tracking issues with work in progress labels Jul 19, 2019

Tug added 5 commits July 19, 2019 18:11

Revert BlockListAppender and InsertionPoint additions

0c7efb4

Fix dismissing block picker

7ca3711

Add missing function in BlockList

c2240cc

Disable add block in HTML mode and show hide keyboard button only whe…

416754e

…n keyboard is shown

Fix bringing back finishInsertingOrReplacingBlock

bd813eb

Tug mentioned this pull request Jul 22, 2019

Move the post title selection state to the store and update PostTitle #16704

Merged

5 tasks

Tug marked this pull request as ready for review July 24, 2019 16:00

Merge remote-tracking branch 'origin/master' into rnmobile/refactor/b…

1d6fa06

…lock-toolbar

Tug requested review from daniloercoli, hypest, marecar3 and mchowning July 25, 2019 17:01

Fix inserting block in first position when post title is selected

5ece15a

Tug added 10 commits July 31, 2019 12:27

Improve defining toolbar height

8532b6f

Merge remote-tracking branch 'origin/master' into rnmobile/refactor/b…

9de00bd

…lock-toolbar

Make html view a flexbax

59f0431

Quickly hide the modal to let the keyboard show up after inserting a …

fb3c590

…new text based block

Let's unmount the modal instead to make sure we don't have timing errors

3713aba

revert to defining the toolbar height in the component itsef

ff28025

Revert "Make html view a flexbax"

d862d1f

This reverts commit 59f0431.

Simplify layout

032ca6d

Fix dismiss keyboard on iOS

42967a5

Merge remote-tracking branch 'origin/master' into rnmobile/refactor/b…

cb940d4

…lock-toolbar

mchowning reviewed Aug 2, 2019

View reviewed changes

mchowning approved these changes Aug 2, 2019

View reviewed changes

Tug merged commit 509da5c into master Aug 5, 2019

Tug deleted the rnmobile/refactor/block-toolbar branch August 5, 2019 11:48

Tug mentioned this pull request Aug 5, 2019

[RNMobile] Refactor BlockToolbar follow-up PR #16906

Merged

5 tasks

pinarol reviewed Aug 6, 2019

View reviewed changes

This was referenced Aug 8, 2019

Fix toolbar bottom inset for iPhone X devices #16961

Merged

Fix toolbar bottom inset for iPhone X devices wordpress-mobile/gutenberg-mobile#1273

Merged

This was referenced Aug 8, 2019

Tech debt: Refactoring of the magic number used in the calculation of extraScrollHeight wordpress-mobile/gutenberg-mobile#1276

Closed

iOS autoscroll not working when typing goes under the keyboard wordpress-mobile/gutenberg-mobile#1275

Closed

youknowriad added this to the Gutenberg 6.3 milestone Aug 9, 2019

snyk-io Bot mentioned this pull request Aug 13, 2024

[Snyk] Upgrade @wordpress/element from 5.28.0 to 6.3.0 Playgirlkaybraz11/wp-xeet#8

Open

Conversation

Tug commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

hypest commented Jul 19, 2019

Uh oh!

Tug commented Jul 19, 2019

Uh oh!

hypest commented Jul 19, 2019

Uh oh!

Tug commented Jul 19, 2019

Uh oh!

hypest commented Jul 19, 2019

Uh oh!

Tug commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hypest commented Jul 19, 2019

Uh oh!

Tug commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tug commented Jul 25, 2019

Uh oh!

mchowning commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tug commented Jul 26, 2019

Uh oh!

mchowning commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tug commented Jul 31, 2019

Uh oh!

mchowning Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tug Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

mchowning left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tug commented Aug 5, 2019

Uh oh!

pinarol Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinarol left a comment

Choose a reason for hiding this comment

Uh oh!

pinarol Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

pinarol Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

etoledom commented Aug 7, 2019

Uh oh!

Tug commented Aug 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Tug commented Jul 19, 2019 •

edited

Loading

Tug commented Jul 19, 2019 •

edited

Loading

Tug commented Jul 24, 2019 •

edited

Loading

mchowning commented Jul 26, 2019 •

edited

Loading

mchowning commented Jul 26, 2019 •

edited

Loading

mchowning Aug 2, 2019 •

edited

Loading

mchowning left a comment •

edited

Loading

pinarol Aug 6, 2019 •

edited

Loading