Add publish functionality to Gutenberg editor #10461

pinarol · 2018-11-15T14:24:15Z

Description

This change adds Publish functionality to the Gutenberg editor. Related issue: wordpress-mobile/gutenberg-mobile#182

Below buttons gain new functionality with this change:

Implementation Details

The basic idea of the implementation is to move common functionality to PostEditorUtil and use it from both GutenbergViewController and AztecPostViewController. So the change set mostly consists of cut/paste from AztecPostViewController to PostEditorUtil. Despite of being big it actually doesn't change a lot since all its doing is moving the logic to another place and solving the compile errors resulting from that. I tested AztecPostViewController by comparing the screen on 2 simulators (one is from master branch the other one is from current branch) and it looks OK but I highly encourage reviewers to do the same king of comparison test.

On the other hand, if this looks risky to you we can just reset everything we did to AztecPostViewController, the Gutenberg side wouldn't be affected from that but it'll cause duplicate logic so I would go with common utility where possible.

Test

Testing prerequisites:

Checkout the PRs branch
clean pods
clean build folder on XCode
run pod install
cd to WordPress-iOS/Gutenberg, run yarn install, yarn start:reset
Run WordPress-iOS app via XCode

I also strongly encourage opening another iOS simulator that runs the code from the master branch. It gives opportunity to compare each part and saving lots of time when we need to decide if sth is the default behavior.

Keep in mind:

currently Publish(Or Update) button is always active on Gutenberg editor unlike Aztec editor. This is done intentionally because Gutenberg side doesn't have the capability of informing every single change yet.
If the post is new or Draft you'll see button label as "Publish", but if you try to edit a published post you 'll see it as "Update". This can be confusing but it is actually a design intent.

Gutenberg tests

Test 1 - Edit published post:

Open one of the published posts
Edit something in the post
Tap Update button
Verify that a loading animation pops
Tap close button
Reopen the post and verify that it is changed

Test 2: Close button

Test 2.1 - Close edited post(which was published before) with "Publish" option:

Open one of the published posts
Edit something in the post
Tap close button
Verify that you see the "You have unsaved changes" alert sheet
Tap Update post
Verify that you see the "Are you sure you want to publish?" alert sheet
Tap "Publish"
Verify that editor is closed and you see an updating animation below the post summary

Test 2.2 - Attempt Close edited post(which was published before) and then "Keep editing":

Open one of the published posts
Edit something in the post
Tap close button
Verify that you see the "You have unsaved changes" alert sheet
Tap "Keep editing"
Verify that alert sheet closes and the content doesn't change

Test 2.3 - Close edited post(which was published before) with "Discard" option:

Open one of the published posts
Edit something in the post
Tap close button
Verify that you see the "You have unsaved changes" alert sheet
Tap "Discard"
Verify that editor is closed and your last changes aren't saved.

Test 2.4 Close un-edited post

Open one of the published posts
Tap close button
Verify that editor is closed without a prompt and nothing changes in the post

Aztec tests

Open EditorSettings.swift in branch try/gutenberg-publish.

Update this line

private var current: Editor { return .gutenberg }

as
private var current: Editor { return .aztec }

Reopen the WordPress-iOS app in try/gutenberg-publish.

Test and compare below cases between WordPress-iOS master and try/gutenberg-publish.

Create a new post and Save as draft
Create a new post and Publish
Edit a draft post and Update
Edit a draft post, tap Close button and Discard
Edit a draft post, tap Close button and Update Draft
Edit a draft post, tap Close button and Keep Editing
Edit a draft post and select navigation bar right button (...)->"Publish Now"
Edit a published post and Update
Edit a published post, tap Close button and Discard
Edit a published post, tap Close button and Update Draft
Edit a published post, tap Close button and Keep Editing
The change doesn't affect this directly but trying also uploading a media, adding a link and other things and publishing would be good.

pinarol · 2018-11-16T11:11:48Z

Note: We should merge the PR wordpress-mobile/gutenberg-mobile#246 as a precondition for merging this PR

wpmobilebot · 2018-11-16T15:13:22Z

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

Generated by 🚫 Danger

pinarol · 2018-11-16T16:00:41Z

@hypest I didn't add you to reviewers since this is highly iOS specific but feel free to contribute please. This is about the publish functionality and you might want to give feedback about "changed" flag. You can go ahead and try some of the test cases and let me know if anything seems wrong. I should say that publishing functionality is very different between iOS and Android in terms of user experience so you might want to checkout an extra WPiOS repo just to see what the default behavior is on master branch.

etoledom

Hey @pinarol - This looks super good! 🎉

I like that the Aztec part looks like no more than moving code to another file, but the execution paths are basically the same. And at the same time we can share that behavior with Gutenberg.

Note:
As we talked by PM, it's not ideal that we are still mixing together behavior and UI. Ideally we will manage to separate concerns clearly, and take this extraction also as an opportunity to make code more testable (and make tests with it). But as a first step thinking on the Alpha release, we agreed that this is the best way to go for now.

I added a couple of observations but most of them are small details. The most important one is concerning requestHTMLReason. I think we should safeguard that in some way, but I'm fine with leaving that task for a future PR.

Regarding behavior test, it looks quite solid 🎉

The Aztec side is doing perfect. 👌
I did find a couple of issues with some corner cases using Gutenberg:

Drafts issue:

Every time I open the editor to create a new post, a local "draft" instance is saved. It should be removed if I didn't save anything when I exist the editor, but it looks like it stays there.

Note that the gif shows the Drafts tab.

Exit without editing:

I noticed that if I add an image to the post, it will always ask me if I want to save before closing the editor, even if I didn't edit anything, or even if I press on Update first.

I'm not sure if this issue is general for both Aztec and Gutenberg. In Aztec there is a known similar issue, so it might not be related to this PR.

If you find out that it's an issue is related to Aztec, or anywhere else that doesn't concern the specific changes made in this PR, I'd advice on leaving it for the future.

etoledom · 2018-11-19T18:49:27Z

WordPress/RN/Gutenberg/GutenbergController.swift

+        }
+    }
+
+    let navigationBarManager = PostEditorNavigationBarManager()


Thank you for the name change! 👍

etoledom · 2018-11-19T18:57:55Z

WordPress/RN/Gutenberg/GutenbergController.swift

+        return PostEditorUtil(context: self)
+    }()
+
+    fileprivate let gutenberg: Gutenberg


Thanks for adding access control to some properties!
I was wondering if fileprivate is required or we can go with private, thinking that we now can access private properties from extensions too.

good reminder 👍 I almost forgot that new behavior about private

etoledom · 2018-11-19T19:17:38Z

WordPress/RN/Gutenberg/GutenbergController.swift

    }

    var isPublishButtonEnabled: Bool {
-        return true
+        return self.postEditorStateContext.isPublishButtonEnabled


Small nitpick: We could clean up self. since it's not really.

yeah, self. here has came via copy/paste.

etoledom · 2018-11-19T19:28:40Z

WordPress/RN/Gutenberg/GutenbergController.swift

@@ -141,6 +219,7 @@ extension GutenbergController: PostEditorNavigationBarManagerDelegate {
    }

    func navigationBarManager(_ manager: PostEditorNavigationBarManager, publishButtonWasPressed sender: UIButton) {
+        requestHTMLReason = .publish


I'm a bit worried about this property holding a state that will later on be used to decide what to do with the html provided by Gutenberg.

I think we can keep it as a first step, but later on it might be dangerous, since if this property changes before Gutenberg returns the html required, it might do something we don't want to.

agreed. it is absolutely not an ideal solution, better solution would be to pass the bridge the reason of html request and take back that reason in the callback. I did this only because the bridge operation only lasted 0.004 seconds, users can't tap another place in this much little time so in terms of practical world I can say that we are safe, at least for now.
but still let's discuss on this and try to find a better solution.

the bridge operation only lasted 0.004 seconds

That is reassuring :)

etoledom · 2018-11-19T19:33:04Z

WordPress/Classes/ViewRelated/Aztec/Helpers/PostEditorUtil.swift

+    }
+
+    struct Analytics {
+        static let editorSource             = "aztec"


Analytics.editorSource is now shared between Aztec and Gutenberg. We will need to make this property dynamic to take into account that.

great catch 👍

This is added to make it similar to Aztec editor

pinarol · 2018-11-20T12:28:50Z

Thanks for your detailed review @etoledom 🎉

I have investigated 2 issues you detected:

Drafts issue: I've fixed this one
Exit without editing when image block is added: I couldn't reproduce the issue but I have a theory about how to fix it. I think this is an intermittent problem resulting from the html parse on Gutenberg side, even if user doesn't make a change, some characters are changed during the parsing on Javascript side. So, once the "changed" flag is fixed by the PR Update the way we determine how a post has been edited gutenberg-mobile#252 I can use that flag to determine this, the fix will be very easy. I have created an issue for this and assigned to myself in order not to forget Gutenberg: Use changed flag on bridge callback on close post #10496

# Conflicts: # Gutenberg # WordPress/Classes/ViewRelated/Aztec/ViewControllers/AztecPostViewController.swift # WordPress/RN/Gutenberg/GutenbergController.swift

pinarol · 2018-11-20T14:39:31Z

I have merged from try/gutenberg so the subrepo Gutenberg isn't used from now on you'll need to remove pods folder and run pod install all over again.

And then you'll need to run yarn start on a separate gutenberg-mobile repo.

etoledom

Looking good! Great job 🎉

* Make a draft solution for gutenberg publish * Handle close button tap & fix lint issues * Update gutenberg mobile subrepo ref * Extract protocol PublishablePostEditor * Set publish button test label from postEditorStateContext * Update gutenberg-mobile subrepo ref * Update gutenberg-mobile subrepo ref to point to master * Revert Podfile.lock updates * Fix some access modifiers and make some line changes * Comment out unnecessary line * Fix code review comments * Fix always creating a new draft even if nothing changes * Add PostCoordinator cancelAnyPending call to init This is added to make it similar to Aztec editor * Remove unnecessary yarn files

pinarol added 5 commits November 15, 2018 00:24

Make a draft solution for gutenberg publish

3d911fb

Handle close button tap & fix lint issues

c3da117

Update gutenberg mobile subrepo ref

fe2c19c

Extract protocol PublishablePostEditor

230f29e

Set publish button test label from postEditorStateContext

90f0cd8

pinarol self-assigned this Nov 15, 2018

pinarol added the [Status] Not Ready for Review label Nov 15, 2018

Tug mentioned this pull request Nov 15, 2018

Send changed to the native bridge if html changed since last time wordpress-mobile/gutenberg-mobile#242

Merged

pinarol added 4 commits November 16, 2018 14:42

Update gutenberg-mobile subrepo ref

9454d81

Update gutenberg-mobile subrepo ref to point to master

a828e30

Revert Podfile.lock updates

e9b1be9

Merge branch 'try/gutenberg' into try/gutenberg-publish

37c07f5

pinarol added 3 commits November 16, 2018 18:31

Fix some access modifiers and make some line changes

bb42d3f

Comment out unnecessary line

1cbee4e

Merge branch 'try/gutenberg' into try/gutenberg-publish

0dcbfe6

pinarol requested review from etoledom and diegoreymendez November 16, 2018 15:54

pinarol added Gutenberg Editing and display of Gutenberg blocks. and removed [Status] Not Ready for Review labels Nov 16, 2018

etoledom reviewed Nov 19, 2018

View reviewed changes

pinarol added 3 commits November 20, 2018 14:26

Fix code review comments

3344919

Fix always creating a new draft even if nothing changes

3b3c46c

Add PostCoordinator cancelAnyPending call to init

97c051c

This is added to make it similar to Aztec editor

Merge branch 'try/gutenberg' into try/gutenberg-publish

3a61d6e

# Conflicts: # Gutenberg # WordPress/Classes/ViewRelated/Aztec/ViewControllers/AztecPostViewController.swift # WordPress/RN/Gutenberg/GutenbergController.swift

Remove unnecessary yarn files

93dd3ab

etoledom approved these changes Nov 20, 2018

View reviewed changes

pinarol merged commit b1cd0ca into try/gutenberg Nov 20, 2018

pinarol deleted the try/gutenberg-publish branch November 20, 2018 16:56

etoledom mentioned this pull request Nov 22, 2018

Gutenberg integration #10520

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add publish functionality to Gutenberg editor #10461

Add publish functionality to Gutenberg editor #10461

pinarol commented Nov 15, 2018 •

edited

Loading

pinarol commented Nov 16, 2018

wpmobilebot commented Nov 16, 2018 •

edited

Loading

pinarol commented Nov 16, 2018

etoledom left a comment

etoledom Nov 19, 2018

etoledom Nov 19, 2018

pinarol Nov 20, 2018

etoledom Nov 19, 2018

pinarol Nov 20, 2018

etoledom Nov 19, 2018

pinarol Nov 20, 2018

etoledom Nov 20, 2018

etoledom Nov 19, 2018

pinarol Nov 20, 2018

pinarol commented Nov 20, 2018

pinarol commented Nov 20, 2018

etoledom left a comment

Add publish functionality to Gutenberg editor #10461

Add publish functionality to Gutenberg editor #10461

Conversation

pinarol commented Nov 15, 2018 • edited Loading

Description

Implementation Details

Test

pinarol commented Nov 16, 2018

wpmobilebot commented Nov 16, 2018 • edited Loading

pinarol commented Nov 16, 2018

etoledom left a comment

Choose a reason for hiding this comment

Drafts issue:

Exit without editing:

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

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

pinarol commented Nov 20, 2018

pinarol commented Nov 20, 2018

etoledom left a comment

Choose a reason for hiding this comment

pinarol commented Nov 15, 2018 •

edited

Loading

wpmobilebot commented Nov 16, 2018 •

edited

Loading