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

Use last gutenberg changes including the editor and block-editor packages refactoring #731

Merged
merged 22 commits into from
Mar 14, 2019

Conversation

Tug
Copy link
Contributor

@Tug Tug commented Mar 11, 2019

Requires WordPress/gutenberg#14375

WordPress/gutenberg#13088 introduced a breaking change for us, we need to update our dependency to the editor store and components as it (the editor package) has been split in 2 packages: core/editor and core/block-editor for more flexibility.

This PR updates our code to make it work with those changes:

  • The main change of this PR is the switch from core/editor to core/block-editor selectors and actions in BlockManager along with the use of BlockEditorProvider in the parent component which makes sure both stores are in sync.

  • In AppContainer, MainApp's is removed and its logic is moved to AppContainer as the component we're using BlockEditorProvider handles defining the SlotFillProvider already.

  • editorSetup() is simplified in src/index.js and most of its logic is moved to edit-post/src/index.native.js. To achieve this we needed to also move UnsupportedBlock to gutenberg.

  • Styles (open for discussion): src/colors.scss and src/variables.(ios|android).scss are replaced with src/_colors.scss and src/_variables.(ios|android).scss and now supersede the files in gutenberg/assets/stylesheets/. This means that, just like with gutenberg, we don't need to explicitly have @import "variables.scss"; or @import "colors.scss"; in our stylesheets. This is closer to what we have on gutenberg web. However it also means that we need to redefine all necessary variables when we decide to have our own version of src/_{file}.scss.

  • @wordpress/block-editor, @wordpress/edit-post and @wordpress/token-list are added to the list of symlinked packages

Testing Instructions

  • Try the demo app on both android and ios and notice that there is no regression
  • Try running the app inside its parent app and notice there is no regression

@etoledom
Copy link
Contributor

Hey @Tug - this is working super good! 🎉
I've been testing on iOS, and so far seems that there are no regressions.

I did find something that I wanted to share early:

It looks like there is a new formatting option on paragraph and heading blocks.
This button doesn't work, but the rest of the buttons work as in develop.

IMG_1461

@Tug
Copy link
Contributor Author

Tug commented Mar 13, 2019

@etoledom Thanks for reviewing. About the new button that's interesting! I didn't notice it :)

Looking at it now 👍

@Tug
Copy link
Contributor Author

Tug commented Mar 13, 2019

So it seems the <code> formatting was added in WordPress/gutenberg#12249 even though it didn't display. For some reason it does now and it seems to work fine at first glance. Since we didn't test it yet I have removed it from our supported formats for now

@etoledom
Copy link
Contributor

Thanks for the update @Tug !

Testing on device it gives me the same error that is shown on the CI tests:
IMG_1462

@Tug
Copy link
Contributor Author

Tug commented Mar 13, 2019

@etoledom Yep, I needed to make an update on gutenberg after the merge, our babel config doesn't support export from

@etoledom
Copy link
Contributor

Thank you @Tug for the update! It looks super good on iOS.
I'm still testing on WPiOS and then Android.

Just wanted to point out that the last commit on
WordPress/gutenberg#14375 seems to break tests there. 🤔

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on iOS example app and WPiOS: ✅ 🎉

I didn't spot any regression or weird behavior.

Tested on Android example app and WPAndroid:

I saw a crash in both example and WP app, not sure if it's related to this or is something else.
The error happened trying to split a paragraph block. It doesn't always happen and I'm not sure about exact steps to reproduce it.

It would be nice if @daniloercoli, @mzorz or @marecar3 could take a look and see if it's related to this PR.

Screenshot_20190313-135411


I give my ✅ for iOS.
Let's wait for the Android side to be verified 👍

Great work as always @Tug ! 🎉

@hypest hypest removed their request for review March 14, 2019 07:20
@daniloercoli
Copy link
Contributor

daniloercoli commented Mar 14, 2019

Tested on Android and was not able to reproduce the problem.
For me this looks good to go, and we can fix the problem on Android later if we notice it again.

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good on the iOS side!
If @daniloercoli is happy with Android, let's :shipit:

@daniloercoli daniloercoli self-requested a review March 14, 2019 14:24
Copy link
Contributor

@daniloercoli daniloercoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Tug Tug merged commit ad19a58 into develop Mar 14, 2019
@Tug Tug deleted the update/gutenberg-editor-refactoring branch March 14, 2019 14:33
@hypest
Copy link
Contributor

hypest commented Mar 14, 2019

Nice work @Tug 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants