Move BlockList and related components to Gutenberg repository #789

Tug · 2019-03-27T12:23:19Z

This PR intends to move src/block-management and its dependencies to gutenberg. It reorganizes the code, refactors components as well as convert Flow syntax to default ES6/JSX syntax.
Context: this is necessary for InnerBlock support

List of changes:

src/block-management/block-manager.js → gutenberg/packages/block-editor/src/components/block-list/index.native.js along with its styles. SafeArea has been moved to src/app/AppContainer and src/app/VisualEditor as we needed a way to extract the post title from the BlockList component. For this we introduce a new prop header which is passed to FlatList as ListHeaderComponent.
src/block-management/block-holder.js → gutenberg/packages/block-editor/src/components/block-list/block.native.js along with its styles
src/block-management/block-picker.js → gutenberg/packages/block-editor/src/components/inserter/index.native.js along with its styles. I also switched to using a selector to get the list of available blocks instead of a global function
src/block-management/block-toolbar.js → gutenberg/packages/block-editor/src/components/block-toolbar/index.native.js along with its styles
src/block-management/inline-toolbar/index.js → gutenberg/packages/block-editor/src/components/block-inspector/index.native.js along with its styles
src/components/keyboard-avoiding-view.*.js → gutenberg/packages/components/src/mobile/keyboard-avoiding-view.*.js
src/components/keyboard-aware-flat-list.*.js → gutenberg/packages/components/src/mobile/keyboard-aware-flat-list.*.js
src/components/readable-content-view.js → gutenberg/packages/components/src/mobile/readable-content-view.native.js
gutenberg/packages/editor/src/components/mobile/bottom-sheet → gutenberg/packages/components/src/mobile/bottom-sheet
gutenberg/packages/editor/src/components/mobile/picker → gutenberg/packages/components/src/mobile/picker
gutenberg/packages/editor/src/components/mobile/unsupported-block → gutenberg/packages/block-library/src/mobile/unsupported-block

Each file was converted from a Flow syntax to standard ES6+JSX format. To visualize the diff you can use the following commands:

git show origin/develop:src/block-management/block-manager.js | git diff --no-index -- - gutenberg/packages/block-editor/src/components/block-list/index.native.js

git show origin/develop:src/block-management/block-holder.js | git diff --no-index -- - gutenberg/packages/block-editor/src/components/block-list/block.native.js

git show origin/develop:src/block-management/block-picker.js | git diff --no-index -- - gutenberg/packages/block-editor/src/components/inserter/index.native.js

git show origin/develop:src/block-management/block-toolbar.js | git diff --no-index -- - gutenberg/packages/block-editor/src/components/block-toolbar/index.native.js

git show origin/develop:src/block-management/inline-toolbar/index.js | git diff --no-index -- - gutenberg/packages/block-editor/src/components/block-inspector/index.native.js

git show origin/develop:src/components/keyboard-avoiding-view.android.js | git diff --no-index -- - gutenberg/packages/components/src/mobile/keyboard-avoiding-view.android.js

git show origin/develop:src/components/keyboard-avoiding-view.ios.js | git diff --no-index -- - gutenberg/packages/components/src/mobile/keyboard-avoiding-view.ios.js

git show origin/develop:src/components/keyboard-aware-flat-list.android.js | git diff --no-index -- - gutenberg/packages/components/src/mobile/keyboard-aware-flat-list.android.js

git show origin/develop:src/components/keyboard-aware-flat-list.ios.js | git diff --no-index -- - gutenberg/packages/components/src/mobile/keyboard-aware-flat-list.ios.js

git show origin/develop:src/components/readable-content-view.js | git diff --no-index -- - gutenberg/packages/components/src/mobile/readable-content-view.native.js

Then check the modifications inside gutenberg only:

git diff --diff-filter=M origin/master...HEAD

Known issues

PostTitle isn't focused on boot. See ed9a548 attempt to fix it.
But even with this fix ^ PostTitle onFocus isn't called when taping on the title, this might get fixed with Fixed issue with re-focusing PostTitle #867
Need to cleanup the styles dependencies Move BlockList and related components to Gutenberg repository #789 (comment)

…list-to-gutenberg

daniloercoli · 2019-04-04T16:23:42Z

Hey @hypest, I checked this PR (mostly run smoke tests) and it seems to work without any issues.
The changes introduced here will break any other open PRs, any idea how we should proceed? Merge it?

peril-wordpress-mobile · 2019-04-05T13:47:07Z

	Warnings
⚠️	PR is missing at least one label.
⚠️	PR is not assigned to a milestone.
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

hypest · 2019-04-05T13:52:39Z

The changes introduced here will break any other open PRs, any idea how we should proceed? Merge it?

👋 @daniloercoli . Thanks for the ping. I think that we should at least try to dive into the code side review a bit and if OK we can merge, but not today (today Apr 5th is a release cut day and too late to bring it into the release).

…list-to-gutenberg

hypest · 2019-04-09T03:59:31Z

src/app/AppContainer.js

+	rootViewHeight: number,
+	safeAreaBottomInset: number,
+	isFullyBordered: boolean,
+}


Feels like the AppContainer has picked up lots of implementation details (wrangling the height of a view for example, wrangling the post title, rendering the visual or html mode) while still having the responsibility to setup the editor and other things.

I wonder if that's too much for a component and it'd make sense to introduce a child component at this stage. Maybe re-introduce a MainScreen component like we had in the past. WDYT?

Sure we could.
I think I took the easiest path for this PR because the layout part is intermixed with the RN bridge subscriptions, a bit of refactoring would help here and I thought it was outside the scope of the PR.

Reason I think it's in scope here is because this PR (in its current form) removes the component separation we already had in place (between the AppContainer and BlockManager) and puts more stuff in the AppContainer. I think it would be best if we could keep AppContainer as "clean" as it was before. Trusted, it wasn't super clean anyway but, no reason to add more to it.

In essence, to introduce the component that will replace our older BlockManager.

Ok, I'll update it ;)

hypest · 2019-04-09T04:07:41Z

src/app/style.scss

+	background-color: #fff;
+}
+
+.blockHolderSemiBordered {


With the BlockHolder now migrated to the Gutenberg side (by utilizing the BlockEdit component), it's weird that there's a style in the mobile repo about it. Can we move it somehow to the Gutenberg side?

I know that this style is used for the post title component too but, this might be the opportunity to reconcile the two?

I'll see if I can reconcile those 👍
Again I went the easy way first for this PR, basically just moving modules around and porting the syntax whenever necessary. I agree there is room for improvement/refactoring but I wonder if we should address those beforehand in this PR or merge rapidly to avoid conflicts and refactor after?

I think it's OK to tackle this one in a separate PR 👍.

Let's add it to a "Known issues" type of section in this PR description so we can help us keep track of it, wdyt?

hypest · 2019-04-09T05:45:20Z

EDIT 2: False alarm. It seems to be an issue with Vysor. When typing directly from the virtual keyboard there is no char duplication. Will try with an external (BT) keyboard as well.
EDIT: the following also happens on develop (at least on hash 061e052) 😢
I think I've noticed an issue with the post title:

Run the demo app on Android
Clear up the title
Type one character
Notice that the char is repeated and ends up being inserted twice

…list-to-gutenberg

Tug · 2019-04-12T13:40:35Z

@hypest I've updated the PR, and tried to split AppContainer in 2. Could you have another look when you have time?

…list-to-gutenberg

hypest · 2019-04-18T10:08:00Z

src/__tests__/html-text-input-test.js

@@ -100,7 +100,7 @@ describe( 'HTMLInputView', () => {

 		const wrapper = shallow(
 			<HTMLInputView
-				setTitleAction={ setTitleAction }
+				editTitle={ setTitleAction }


Will this prop set override the editTitle provided inside the HTMLInputView itself via withDispatch?

Yes because we're not importing the default HTMLInputView component here but the original class exported here

Ah, I see. Gosh, that pattern is super confusing :(. I wish we could make it better.

hypest · 2019-04-18T10:09:17Z

src/components/html-text-input.js

@@ -28,6 +28,7 @@ type PropsType = {
 	setTitleAction: string => void,


The setTitleAction doesn't seem to be used anymore, can we remove it?

Indeed 👍
(weird that flow didn't report it)

This reverts commit ed9a548.

hypest · 2019-04-18T10:33:51Z

src/app/AppContainer.js


-		const hasChanges = title !== this.lastTitle || html !== this.lastHtml;
+		const hasChanges = title !== this.post.title.raw || html !== this.post.content.raw;


I wonder, is the serialization/parsing pair symmetric nowadays? I remember we needed the this.lastHtml = serialize( parse( props.initialHtml || '' ) ) dance because the pair wasn't symmetric. Have you perhaps tried backing out of the block editor in WPAndroid and the post is not marked as changes if no user edits occurred?

I haven't tried but I can have a look with our sample post (initial-html.js)

src/app/AppContainer.js

koke · 2019-05-03T10:58:09Z

I started testing from WPiOS and got this error:

Create a new post
~~Type a title and press enter~~
Type some paragraph text and press enter
Tap on the inserter button

ExceptionsManager.js:74 Invariant Violation: [715,"RCTView",{"paddingBottom":"<<NaN>>"}] is not usable as a native method argument

This error is located at:
    in RCTView (at View.js:45)
    in View (at KeyboardAvoidingView.js:207)
    in KeyboardAvoidingView (at keyboard-avoiding-view.ios.js:12)
    in KeyboardAvoidingView (at bottom-sheet/index.native.js:138)
    in RCTView (at View.js:45)
    in View (at createAnimatedComponent.js:151)
    in AnimatedComponent (at createAnimatableComponent.js:571)
    in withAnimatable(View) (at src/index.js:437)
    in RCTView (at View.js:45)
    in View (at AppContainer.js:98)
    in RCTView (at View.js:45)
    in View (at AppContainer.js:115)
    in AppContainer (at Modal.js:244)
    in RCTView (at View.js:45)
    in View (at Modal.js:264)
    in RCTModalHostView (at Modal.js:252)
    in Modal (at src/index.js:450)
    in ReactNativeModal (at bottom-sheet/index.native.js:121)
    in BottomSheet (at inserter/index.native.js:26)
    in Inserter (at with-select/index.js:187)
    in ComponentWithSelect (at with-select/index.js:196)
    in WithSelect(Inserter) (at block-list/index.native.js:191)
    in BlockList (at with-dispatch/index.js:122)
    in ComponentWithDispatch (at with-dispatch/index.js:129)
    in WithDispatch(BlockList) (at with-select/index.js:187)
    in ComponentWithSelect (at with-select/index.js:196)
    in WithSelect(WithDispatch(BlockList)) (at VisualEditor.js:83)
    in SlotFillProvider (at provider/index.native.js:130)
    in BlockEditorProvider (at provider/index.native.js:21)
    in WithRegistry(BlockEditorProvider) (at with-dispatch/index.js:122)
    in ComponentWithDispatch (at with-dispatch/index.js:129)
    in WithDispatch(WithRegistry(BlockEditorProvider)) (at VisualEditor.js:77)
    in VisualEditor (at with-dispatch/index.js:122)
    in ComponentWithDispatch (at with-dispatch/index.js:129)
    in WithDispatch(VisualEditor) (at with-select/index.js:187)
    in ComponentWithSelect (at with-select/index.js:196)
    in WithSelect(WithDispatch(VisualEditor)) (at MainScreen.js:115)
    in RCTSafeAreaView (at SafeAreaView.js:45)
    in SafeAreaView (at MainScreen.js:130)
    in MainScreen (at with-select/index.js:187)
    in ComponentWithSelect (at with-select/index.js:196)
    in WithSelect(MainScreen) (at AppContainer.js:190)
    in AppContainer (at with-dispatch/index.js:122)
    in ComponentWithDispatch (at with-dispatch/index.js:129)
    in WithDispatch(AppContainer) (at with-select/index.js:187)
    in ComponentWithSelect (at with-select/index.js:196)
    in WithSelect(WithDispatch(AppContainer)) (at App.js:33)
    in AppProvider (at src/index.js:69)
    in RootComponent (at renderApplication.js:35)
    in RCTView (at View.js:45)
    in View (at AppContainer.js:98)
    in RCTView (at View.js:45)
    in View (at AppContainer.js:115)
    in AppContainer (at renderApplication.js:34)

koke · 2019-05-03T11:19:32Z

I don't understand why InlineToolbar has moved to BlockInspector, since those are two different things. The only reason I can think of is that InspectorControls.Slot was previously inside the inline toolbar, which also doesn't make much sense to me.

koke · 2019-05-03T11:37:21Z

Beyond the two previous comments, I think this PR is just too big to review. It's good as a proof of concept to know how to do this, but instead of trying to keep up with constant changes while trying to review, I'd break this down in several steps. The most important idea is to avoid moving a file to a different repo and editing it at the same time, since it makes it really hard to evaluate the changes.

The end result of this would be that pretty much all code, except the initialization and app container, would be inside Gutenberg. Since we'll have to remove the flow types for any code we move, I think we should start by removing Flow completely within gutenberg-mobile before we move code (cc @hypest).

I imagine these PRs:

Remove flow from the project and edit any files necessary to remove flow annotations. If we can disable flow per-file, I'd be fine with that, but it doesn't make sense to me to keep flow at this stage. After this PR, we should be able to freely move any file to the Gutenberg repo without changes.
Move every component that was moved in this PR to packages/mobile as is, with no changes, except whatever necessary to make the code work (but no API changes).
Port BlockPicker to Inserter
Port BlockToolbar
Port BlockHolder / InlineToolbar
Port BlockManager to BlockList
(Bonus) Move VisualEditor to Gutenberg. Maybe we want to use initializeEditor as the container like the web, but that's beyond the scope for the block list.

I'd also suggest that any of the mobile-only components should have it's own folder with a README.md.

hypest · 2019-05-03T12:35:56Z

we should start by removing Flow completely within gutenberg-mobile before we move code (cc @hypest).

👋 @koke , can you expand on why that'd be a necessary step? Is it perhaps so to be super easy to compare if a file moved in an as pristine form as possible? If that's the reason, I think that with small PRs it won't be hard to review the changes, including the removal of flow markup of the files in each PR.

Basically, I'd prefer if we do not remove flow from the gutenberg-mobile repo, at least not until the only things left in this repo are super trivial. Happy to revise that opinion if removing flow is an actual necessary first step.

By the way, it would be nice if we had some adjustment in processes or tooling (editors/IDEs) to make up for the loss of flowtypes in the Gutenberg repo.

koke · 2019-05-03T13:30:48Z

I've been learning a bit more about Flow after that comment, and I don't think we need to remove it, as dropping the @flow declaration from each individual file would be enough.

The goal is to avoid as much as possible having changes to files as they move to another repo. So we would drop the @flow from a file, remove the type annotations, get that merged, then do a PR that just moves the files.

It looks like actually removing flow completely from the project is quite a bit of trouble since that's somewhat coupled to the react native defaults, so I'm fine leaving it, even if we don't use it much.

Tug · 2019-06-06T20:12:02Z

Closing this PR. Progress of this is now being monitored on #958

Move BlockList and related components to Gutenberg repository

a8acbef

Tug added the [Status] Not Ready For Review label Mar 27, 2019

Tug self-assigned this Mar 27, 2019

Tug mentioned this pull request Mar 27, 2019

Native support for BlockList WordPress/gutenberg#14662

Closed

Tug added 4 commits March 27, 2019 18:20

Merge remote-tracking branch 'origin/develop' into update/move-block-…

203def2

…list-to-gutenberg

Fix tests

d721eed

Fix lint errors

b2fd526

Remove SafeArea for the HTML editor

1854229

Tug force-pushed the update/move-block-list-to-gutenberg branch from 3b6bdd2 to 1854229 Compare March 28, 2019 12:58

Update gutenberg ref

e567f58

Tug removed the [Status] Not Ready For Review label Mar 28, 2019

Tug added 2 commits March 28, 2019 15:04

Merge remote-tracking branch 'origin/develop' into update/move-block-…

2a5848f

…list-to-gutenberg

Merge remote-tracking branch 'origin/develop' into update/move-block-…

b2d6c1e

…list-to-gutenberg

Tug mentioned this pull request Apr 4, 2019

Media text block #689

Closed

Get latest changes from gb mobile

07075ee

Tug added 2 commits April 5, 2019 17:05

Merge remote-tracking branch 'origin/develop' into update/move-block-…

355b03d

…list-to-gutenberg

Update gutenberg ref

cf7a578

hypest reviewed Apr 9, 2019

View reviewed changes

Tug added 5 commits April 10, 2019 13:15

Merge remote-tracking branch 'origin/develop' into update/move-block-…

e051a59

…list-to-gutenberg

Merge remote-tracking branch 'origin/develop' into update/move-block-…

94067e2

…list-to-gutenberg

Extract VisualEditor from AppContainer

c0652c9

Update gutenberg ref

0ec70e4

Merge remote-tracking branch 'origin/develop' into update/move-block-…

b842900

…list-to-gutenberg

mkevins mentioned this pull request Apr 13, 2019

[Mobile] Fix caret position after multi line paste WordPress/gutenberg#14974

Closed

5 tasks

mkevins mentioned this pull request Apr 13, 2019

Fix caret position after multi line paste #870

Closed

Tug added 2 commits April 15, 2019 11:23

Merge remote-tracking branch 'origin/develop' into update/move-block-…

b8e55cd

…list-to-gutenberg

Add MainScreen component that deals with switching the type of editor

763a396

Tug force-pushed the update/move-block-list-to-gutenberg branch from 0b4de7b to 763a396 Compare April 16, 2019 08:52

Tug added 4 commits April 16, 2019 10:59

Remove unused StateType in AppContainer

e88e4a6

Fix focus on title on mount

ed9a548

Merge remote-tracking branch 'origin/develop' into update/move-block-…

66d6f26

…list-to-gutenberg

Merge remote-tracking branch 'origin/develop' into update/move-block-…

e762945

…list-to-gutenberg

Tug mentioned this pull request Apr 18, 2019

[GB Mobile] Move BlockList and related components to Gutenberg repository wordpress-mobile/WordPress-Android#9663

Closed

hypest reviewed Apr 18, 2019

View reviewed changes

Add back ReadableContentView around PostTitle

4a89fb1

hypest reviewed Apr 18, 2019

View reviewed changes

Revert "Fix focus on title on mount"

0563a76

This reverts commit ed9a548.

hypest reviewed Apr 18, 2019

View reviewed changes

Tug added 3 commits April 18, 2019 12:44

Remove unused prop definition setTitleAction

7010a2a

Reduce diff size by reordering orders of methods in AppContainer

e6c2571

Fix post modified when opened and closed

9ab0d9a

hypest reviewed Apr 18, 2019

View reviewed changes

src/app/AppContainer.js Show resolved Hide resolved

Revert to old logic and add comment explaining why it is there

619f72d

This was referenced May 6, 2019

Convert BlockManager into BlockList #958

Closed

Remove flow from the files that will go to Gutenberg #960

Closed

Tug mentioned this pull request May 21, 2019

[RNMobile] Master thread: Native support for inner blocks WordPress/gutenberg#15749

Closed

8 tasks

Tug closed this Jun 6, 2019

Tug deleted the update/move-block-list-to-gutenberg branch June 6, 2019 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move BlockList and related components to Gutenberg repository #789

Move BlockList and related components to Gutenberg repository #789

Tug commented Mar 27, 2019 •

edited

Loading

daniloercoli commented Apr 4, 2019

peril-wordpress-mobile bot commented Apr 5, 2019

hypest commented Apr 5, 2019

hypest Apr 9, 2019

Tug Apr 10, 2019

hypest Apr 10, 2019

Tug Apr 10, 2019 •

edited

Loading

hypest Apr 9, 2019

Tug Apr 10, 2019

hypest Apr 10, 2019

hypest commented Apr 9, 2019 •

edited

Loading

Tug commented Apr 12, 2019

hypest Apr 18, 2019

Tug Apr 18, 2019

hypest Apr 18, 2019

hypest Apr 18, 2019

Tug Apr 18, 2019

hypest Apr 18, 2019 •

edited

Loading

Tug Apr 18, 2019

koke commented May 3, 2019 •

edited

Loading

koke commented May 3, 2019

koke commented May 3, 2019 •

edited

Loading

hypest commented May 3, 2019

koke commented May 3, 2019

Tug commented Jun 6, 2019

		@@ -28,6 +28,7 @@ type PropsType = {
		setTitleAction: string => void,


		const hasChanges = title !== this.lastTitle \|\| html !== this.lastHtml;
		const hasChanges = title !== this.post.title.raw \|\| html !== this.post.content.raw;

Move BlockList and related components to Gutenberg repository #789

Move BlockList and related components to Gutenberg repository #789

Conversation

Tug commented Mar 27, 2019 • edited Loading

List of changes:

Known issues

daniloercoli commented Apr 4, 2019

peril-wordpress-mobile bot commented Apr 5, 2019

hypest commented Apr 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tug Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest commented Apr 9, 2019 • edited Loading

Tug commented Apr 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koke commented May 3, 2019 • edited Loading

koke commented May 3, 2019

koke commented May 3, 2019 • edited Loading

hypest commented May 3, 2019

koke commented May 3, 2019

Tug commented Jun 6, 2019

Tug commented Mar 27, 2019 •

edited

Loading

Tug Apr 10, 2019 •

edited

Loading

hypest commented Apr 9, 2019 •

edited

Loading

hypest Apr 18, 2019 •

edited

Loading

koke commented May 3, 2019 •

edited

Loading

koke commented May 3, 2019 •

edited

Loading