Skip to content

Media & Text - Media picker buttons functionality#1378

Merged
koke merged 20 commits intodevelopfrom
feature/media-text-improvements
Oct 10, 2019
Merged

Media & Text - Media picker buttons functionality#1378
koke merged 20 commits intodevelopfrom
feature/media-text-improvements

Conversation

@geriux
Copy link
Copy Markdown
Contributor

@geriux geriux commented Sep 23, 2019 •
edited
Loading

Gutenberg PR WordPress/gutenberg#17537

Fixes #1313

To test check WordPress/gutenberg#17537 for a full description and screenshots.

It also includes a small change in RNReactNativeGutenbergBridge.swift to pass the media type.

Parent apps PRs

WPiOS: wordpress-mobile/WordPress-iOS#12581
WPAndroid: wordpress-mobile/WordPress-Android#10567 (it has the changes from the multiple media PR) but there aren't any new native changes, just the updated gutenberg reference

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@geriux geriux mentioned this pull request Sep 23, 2019
Merged
5 tasks
@geriux geriux marked this pull request as ready for review September 24, 2019 08:46
@geriux geriux requested a review from koke September 24, 2019 08:47
@koke
Copy link
Copy Markdown
Member

koke commented Sep 25, 2019

I tried to upload a video and after the upload finished, I got this warning:

Failed to get size for image: https://sandbox.koke.me/wp-content/uploads/2019/09/img_1624.mov

And the video part in media-text only shows the placeholder.

@geriux
Copy link
Copy Markdown
Contributor Author

geriux commented Sep 25, 2019

I tried to upload a video and after the upload finished, I got this warning:

Failed to get size for image: https://sandbox.koke.me/wp-content/uploads/2019/09/img_1624.mov

And the video part in media-text only shows the placeholder.

Oh no, that's weird, do you know if I have to do something for the change of ios/RNReactNativeGutenbergBridge.swift to work? Did you test it locally? If so, you would have to rebuild because of the native change.

@koke
Copy link
Copy Markdown
Member

koke commented Sep 25, 2019

If so, you would have to rebuild because of the native change

🤦‍♂ My bad, I didn't realize there were native changes and was testing with the wrong binaries

@koke
Copy link
Copy Markdown
Member

koke commented Sep 25, 2019

I got a weird issue... after testing the video upload, I removed the media-text block, inserted a new one and tried to upload an image. The media-text block will upload correctly but then try to show the image as a video and fail. When I closed the editor I got a crash:

Unhandled JS Exception: Cannot read property 'props' of null, stack:
<unknown>@177709:33
<unknown>@40386:21
_callTimer@40303:9
Object.callTimers@40510:9
MessageQueue.__callFunction@15816:44
<unknown>@15558:17
MessageQueue.__guard@15770:13
MessageQueue.callFunctionReturnFlushedQueue@15557:14
<unknown>@80:58

I haven't been able to reproduce the crash again 😞

@koke
Copy link
Copy Markdown
Member

koke commented Sep 25, 2019

@177709:33

Looking at the generated JS, the crash seems to be in KeyboardAwareFlatList, so I would assume it's a coincidence unrelated to these changes, but let me know if you think you changed something that might be causing that.

@geriux
Copy link
Copy Markdown
Contributor Author

geriux commented Sep 25, 2019

@177709:33

Looking at the generated JS, the crash seems to be in KeyboardAwareFlatList, so I would assume it's a coincidence unrelated to these changes, but let me know if you think you changed something that might be causing that.

Not really, but I will double check just in case

@pinarol pinarol added Media [Type] Enhancement Improves a current area of the editor labels Sep 26, 2019
@pinarol pinarol added this to the 1.13 milestone Sep 26, 2019
@geriux geriux mentioned this pull request Sep 30, 2019
Merged
1 task
@etoledom etoledom modified the milestones: 1.13, 1.15 Oct 2, 2019
@etoledom
Copy link
Copy Markdown
Contributor

etoledom commented Oct 2, 2019

Milestone moved to 1.15

geriux added 2 commits October 3, 2019 21:27
@geriux
Merge branch 'develop' of github.com:wordpress-mobile/gutenberg-mobil…
a154f5c 
…e into feature/media-text-improvements

# Conflicts:
#	gutenberg
#	ios/gutenberg/GutenbergViewController.swift
#	react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/ReactNativeGutenbergBridge/GutenbergBridgeJS2Parent.java
#	react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/ReactNativeGutenbergBridge/RNReactNativeGutenbergBridgeModule.java
#	react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java
#	react-native-gutenberg-bridge/ios/GutenbergBridgeDelegate.swift
#	react-native-gutenberg-bridge/ios/RNReactNativeGutenbergBridge.swift
@geriux
Update to fork
9e7b2e8
@geriux geriux mentioned this pull request Oct 3, 2019
Merged
2 tasks
@mkevins mkevins mentioned this pull request Oct 4, 2019
Closed
2 tasks
@koke koke requested review from mkevins and removed request for mkevins October 4, 2019 08:44
@mkevins mkevins mentioned this pull request Oct 9, 2019
Closed
@koke koke merged commit de6141a into develop Oct 10, 2019
etoledom
etoledom reviewed Oct 11, 2019
View reviewed changes
react-native-gutenberg-bridge/ios/GutenbergBridgeDelegate.swift
@@ -1,4 +1,4 @@
public typealias MediaPickerDidPickMediaCallback = (_ media: [(Int32?,String?)]?) -> Void
public typealias MediaPickerDidPickMediaCallback = (_ media: [(Int32?,String?,String?)]?) -> Void
Copy link
Copy Markdown
Contributor

@etoledom etoledom Oct 11, 2019

Choose a reason for hiding this comment

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

These callback parameter list is kind of growing a lot 🤔 . Maybe we could think about using a struct?
Having too many unnamed parameters in the public API doesn't sound like a good practice.
cc @koke @pinarol

Copy link
Copy Markdown
Member

@koke koke Oct 14, 2019

Choose a reason for hiding this comment

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

That sounds like a good idea to me 😄

@pinarol pinarol deleted the feature/media-text-improvements branch October 14, 2019 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@koke koke koke approved these changes

+1 more reviewer

@etoledom etoledom etoledom left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@geriux geriux

Labels

Media [Type] Enhancement Improves a current area of the editor

Projects

None yet

Milestone

1.15

Development

Successfully merging this pull request may close these issues.

Media & Text - Media picker buttons are not functional

4 participants

@geriux @koke @etoledom @pinarol