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

Media & Text - Media picker buttons functionality #1378

Merged
merged 20 commits into from
Oct 10, 2019

Conversation

geriux
Copy link
Contributor

@geriux geriux commented Sep 23, 2019

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 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
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
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
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
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
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
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
@etoledom etoledom modified the milestones: 1.13, 1.15 Oct 2, 2019
@etoledom
Copy link
Contributor

etoledom commented Oct 2, 2019

Milestone moved to 1.15

geriux added 2 commits October 3, 2019 21:27
…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
@koke koke requested review from mkevins and removed request for mkevins October 4, 2019 08:44
@koke koke merged commit de6141a into develop Oct 10, 2019
@@ -1,4 +1,4 @@
public typealias MediaPickerDidPickMediaCallback = (_ media: [(Int32?,String?)]?) -> Void
public typealias MediaPickerDidPickMediaCallback = (_ media: [(Int32?,String?,String?)]?) -> Void
Copy link
Contributor

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
Member

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
Labels
Media [Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Media & Text - Media picker buttons are not functional
4 participants