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

Refactor BlockToolbar out of BlockList #16677

Merged
merged 24 commits into from Aug 5, 2019

Conversation

@Tug
Copy link
Contributor

commented Jul 19, 2019

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.
Tug added 2 commits Jul 17, 2019
Remove BlockEditorProvider from BlockList and add native version of E…
…ditorProvider to Editor. Plus support InsertionPoint and BlockListAppender
@hypest

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

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.

@Tug

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

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

@Tug Tug marked this pull request as ready for review Jul 24, 2019

@Tug Tug requested review from mchowning, hypest, marecar3 and daniloercoli Jul 25, 2019

@Tug

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

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:

  1. 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)
  2. resolved - There is no ABH indicator when inserting a post from the post title (recent PR adding this)
  3. 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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

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:

  1. 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)
  2. 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.

@mchowning

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

Did a quick follow-up test with the latest changes, and I noticed two issues around adding blocks, neither of which I saw on develop. 😞

1. Keyboard dismisses when adding blocks (Android only)

When adding a new block on Android the keyboard is dismissing even when the new block is text-based.

Steps:

  1. Select a text-based block (i.e. header bock)
  2. Observe the keyboard is open
  3. Tap the and add a new Header block
  4. Observe the keyboard is closed

Note: Tapping enter to add a new block instead of using the works fine.

2. Focus reverts back to previous block (iOS only)

When adding a new block on iOS, the focus goes to the new block and then immediately jumps back to the previously selected block.

Screen Capture on 2019-07-30 at 09-29-50

Steps

  1. Select any text block (or title)
  2. Tap and add a new text block
  3. Observe the cursor briefly appears in the newly added block and then immediately returns to the previously selected block
@Tug

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

@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 :)


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

This comment has been minimized.

Copy link
@mchowning

mchowning Aug 2, 2019

Contributor

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.

This comment has been minimized.

Copy link
@Tug

Tug Aug 5, 2019

Author Contributor

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
Copy link
Contributor

left a comment

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

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

@Tug

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@mchowning Thanks a lot for the review 🙇

@Tug Tug merged commit 509da5c into master Aug 5, 2019

1 of 4 checks passed

Filter opened Filter opened
Details
Filter opened Filter opened
Details
Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details

@Tug Tug deleted the rnmobile/refactor/block-toolbar branch Aug 5, 2019

@Tug Tug referenced this pull request Aug 5, 2019
5 of 5 tasks complete
innerToolbarHeight={ blockMobileToolbarHeight }
safeAreaBottomInset={ this.props.safeAreaBottomInset }
parentHeight={ this.props.rootViewHeight }
extraScrollHeight={ innerToolbarHeight + 10 }

This comment has been minimized.

Copy link
@pinarol

pinarol Aug 6, 2019

Contributor

what does "+10" refer to?

@pinarol
Copy link
Contributor

left a comment

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

@@ -169,10 +101,7 @@ export class BlockList extends Component {
{ ...( Platform.OS === 'android' ? { removeClippedSubviews: false } : {} ) } // Disable clipping on Android to fix focus losing. See https://github.com/wordpress-mobile/gutenberg-mobile/pull/741#issuecomment-472746541
accessibilityLabel="block-list"
innerRef={ this.scrollViewInnerRef }
blockToolbarHeight={ toolbarHeight }
innerToolbarHeight={ blockMobileToolbarHeight }
safeAreaBottomInset={ this.props.safeAreaBottomInset }

This comment has been minimized.

Copy link
@pinarol

pinarol Aug 6, 2019

Contributor

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

@@ -169,10 +101,7 @@ export class BlockList extends Component {
{ ...( Platform.OS === 'android' ? { removeClippedSubviews: false } : {} ) } // Disable clipping on Android to fix focus losing. See https://github.com/wordpress-mobile/gutenberg-mobile/pull/741#issuecomment-472746541
accessibilityLabel="block-list"
innerRef={ this.scrollViewInnerRef }
blockToolbarHeight={ toolbarHeight }
innerToolbarHeight={ blockMobileToolbarHeight }

This comment has been minimized.

Copy link
@pinarol

pinarol Aug 6, 2019

Contributor

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

@etoledom

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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

safe_area

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

@Tug

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

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 👍

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

gziolo added a commit that referenced this pull request Aug 29, 2019
Refactor BlockToolbar out of BlockList (#16677)
* 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
gziolo added a commit that referenced this pull request Aug 29, 2019
Refactor BlockToolbar out of BlockList (#16677)
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.