Skip to content

Conversation

@pinarol
Copy link
Contributor

@pinarol pinarol commented Nov 8, 2018

Fixes wordpress-mobile/gutenberg-mobile#192

This has child PRs so we need to merge them first:
wordpress-mobile/gutenberg-mobile#229 (merged)
WordPress/gutenberg#11635 (merged)
WordPress/gutenberg#11660 (merged)

To test:

  • Clone the WordPress-iOs project, checkout branch try/gutenberg-mediapicker.
  • git submodule init --update (clone this project as submodule of WPiOS)
  • cd Gutenberg -> This project (and not gutenberg-web)
  • All the normal steps to make gutenberg-mobile work. (yarn install... yarn start etc...)
  • Go to the WPiOS root folder and pod install.
  • Build and run the project on XCode.

Test 1:

  • Open a post, add an image block
  • Tap Media library and select an image
  • Verify that the image block is updated as the selected image

Test 2:

  • Open a post, add an image block
  • Tap Media library and tap "Cancel"
  • Verify that picker is closed
  • Tap Media library and select image
  • Verify that the image block is updated as the selected image

Test 3:
Please refer to child PR to find specific test cases related to toolbar.

Test 4:(Testing example app)
There's an example app under: WordPress-iOS/Gutenberg/ios/gutenberg.xcodeproj it should also work ok after this PR. It should be run by "yarn ios"

Open RCTAztecViewManager.m
Update it as
#import "RNTAztecView-Swift.h"
//#import "WordPress-Swift.h"

  • stop metro if that was already running
  • cd to WordPress-iOS/Gutenberg
  • run yarn start:reset, yarn ios
  • Example app opens, tap Media Library on it
  • Verify that image block is updated with a landscape image

@pinarol pinarol requested a review from etoledom November 8, 2018 15:52
@pinarol pinarol requested a review from hypest November 8, 2018 16:46
@pinarol
Copy link
Contributor Author

pinarol commented Nov 9, 2018

I updated the react-native-aztec subrepo ref with the last commit, so following lines will allow you to test the code with XCode via WPiOS by default:

// USE "WordPress-Swift.h" to run on WPiOS, or "RNTAztecView-Swift.h" to run Example App.
//#import "RNTAztecView-Swift.h"
#import "WordPress-Swift.h"

If you want to run the example app via "yarn ios" you should switch the imports here

@pinarol
Copy link
Contributor Author

pinarol commented Nov 9, 2018

I included the child PR about the image block toolbar WordPress/gutenberg#11660 into this PR with the last commit because it has very similar test steps with this PR so same person can easily test.

@pinarol
Copy link
Contributor Author

pinarol commented Nov 9, 2018

I added a new commit to fix the example app.

To test that you should refer to Test 4.

@hypest
Copy link
Contributor

hypest commented Nov 9, 2018

Tested:

  • select image from media library ✅
  • cancel selecting ✅
  • noticing toolbar edit button appearing or disappearing depending on Image block state ✅
  • selecting a new image from the library on toolbar button press ✅

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Nov 9, 2018

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

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.

It's looking super good! 🎉

I had one small observation and one question (out of curiosity mostly):

GutenbergMediaPickerHelper looks super similar to the AztecPostViewController media picker handling. I can see small differences but I think that's because of this simplified first implementation.
Do you think that GutenbergMediaPickerHelper could grow enough to be shared between AztecPostViewController and GutenbergController?

I know I'm going a bit (a lot) ahead of time.
I'm happy to merge GutenbergMediaPickerHelper as it is now 👍


let navBarManager = PostEditorNavigationBarManager()

var mediaPickerHelper: GutenbergMediaPickerHelper!
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can use a lazy var instead of a❗️optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Much better now :)

@pinarol
Copy link
Contributor Author

pinarol commented Nov 9, 2018

Do you think that GutenbergMediaPickerHelper could grow enough to be shared between AztecPostViewController and GutenbergController?

Yes absolutely, once gutenberg has the same(or similar enough) media picker functionality with aztec we can extract all related code to GutenbergMediaPickerHelper and make them both use it. in this case we'd rename it also. Of course it depends on how much functionality they will share but we should definitely look for ways to decrease code duplication.

@pinarol
Copy link
Contributor Author

pinarol commented Nov 12, 2018

Let's get #10441 merged first so that we won't need the hacky fix to make this CI builds pass and we can get rid of branch react-native-aztec/temp-import-for-react-native.

@pinarol
Copy link
Contributor Author

pinarol commented Nov 13, 2018

We can merge this after merging wordpress-mobile/gutenberg-mobile#229

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.

Working great! 🎉

Ready to merge when BB is happy.

Nice work @pinarol

@pinarol pinarol merged commit aad9eaf into try/gutenberg Nov 13, 2018
@pinarol pinarol deleted the try/gutenberg-mediapicker branch November 13, 2018 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants