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

Rnmobile/upload media file #13128

Merged
merged 56 commits into from Jan 28, 2019

Conversation

@marecar3
Copy link
Contributor

marecar3 commented Dec 28, 2018

Description

This is the supporting PR for upload image file from react native Gutenberg editor

How has this been tested?

This PR can be tested from WPAndroid PR

Screenshots

ezgif com-video-to-gif 1

mzorz and others added some commits Dec 20, 2018

Transform ImageEdit from function to class
Add upload media file callback
Show spinner and opacity when image is uploading

@marecar3 marecar3 referenced this pull request Dec 28, 2018

Merged

Gb/upload media file #8879

@marecar3 marecar3 requested review from Tug , koke and etoledom Dec 28, 2018

@marecar3 marecar3 self-assigned this Dec 28, 2018

@mzorz
Copy link
Contributor

mzorz left a comment

Again nice job @marecar3 , left some comments!

style={ { textAlign: 'center' } }
underlineColorAndroid="transparent"
value={ caption }
placeholder={ 'Write caption…' }

This comment has been minimized.

@mzorz

mzorz Jan 4, 2019

Contributor

This could be passed through i18n, like _( 'Write caption...' )

This comment has been minimized.

@marecar3

marecar3 Jan 16, 2019

Author Contributor

Thanks @mzorz ! Fixed and pushed.

{ toolbarEditButton }
</BlockControls>
<Image
style={ { width: '100%', height: 200, opacity: opacity } }

This comment has been minimized.

@mzorz

mzorz Jan 4, 2019

Contributor

I can see this value was there before this change, but wondering why height: 200, is there anything specific to that value?

This comment has been minimized.

@marecar3

marecar3 Jan 16, 2019

Author Contributor

Maybe we can ask @etoledom about this one, as this change was made a long time ago?

This comment has been minimized.

@marecar3

marecar3 Jan 16, 2019

Author Contributor

I saw that this was changed with this PR #13096
so @etoledom , you can ignore above question :) Thanks.

const toolbarEditButton = (
<Toolbar>
<ToolbarButton
className="components-toolbar__control"

This comment has been minimized.

@mzorz

mzorz Jan 4, 2019

Contributor

is there a reason for having a double underscore "__" in this name? should it be a normal dash?

This comment has been minimized.

@koke

koke Jan 14, 2019

Contributor

It is part of the CSS coding standards for Gutenberg. I would ask if we actually need the className in the native implementation

This comment has been minimized.

@marecar3

marecar3 Jan 16, 2019

Author Contributor

Yeah, thanks @koke. We don't need that one, I removed it.

This comment has been minimized.

@mzorz

mzorz Jan 21, 2019

Contributor

It is part of the CSS coding standards for Gutenberg.

TIL - thanks @koke

Show resolved Hide resolved packages/block-library/src/image/edit.native.js
);

const http = 'http';
const showSpinner = url !== undefined ? ! url.includes( http ) : false;

This comment has been minimized.

@mzorz

mzorz Jan 4, 2019

Contributor

can we perhaps extend this logic to a more generic Utils class or if not, at least method on its own? I'm thinking something more explicit like isRemoteUrl() that hides the logic inside to determine whether an URI is a remote resource or not, etc. Such const http constant should belong within the scope of such function only, etc.

This comment has been minimized.

@koke

koke Jan 14, 2019

Contributor

You can import { isURL } from '@wordpress/url' 😉

https://github.com/WordPress/gutenberg/blob/master/packages/url/README.md

This comment has been minimized.

@marecar3

marecar3 Jan 16, 2019

Author Contributor

Thanks @mzorz , @koke . I have updated the code to use isURL method like @koke suggested.

@etoledom
Copy link
Contributor

etoledom left a comment

Hey @marecar3 ! Thank you for your great work!

I like the idea of leaving all the networking to the parents app, while we use Gutenberg to show the upload state to the user. 👌

I tried a fast implementation on the iOS side of media upload to see if we could have any compatibility issue. Sadly I did found a complication that didn't let me finish it. I explained that on a code comment.

Apart of that it looks good! I also added a small clean up comment but I don't think that is a blocking thing.

One more comment: I was working on another PR (#13096) that modifies the image/edit.native.js file quite a lot too. I think that we will have a big merge conflict there. Let's chat to see what we can do about it. 😛

}

addMediaUploadListener( mediaId ) {
gutenbergBridgeEvents.addListener( 'mediaUpload' + mediaId, this.mediaUpload );

This comment has been minimized.

@etoledom

etoledom Jan 7, 2019

Contributor

I believe we will have a problem here with iOS. As you can see here, on iOS we need to know ahead of time what events do we support by overriding - (NSArray<NSString *> *)supportedEvents and returning an array with the event names.

Because of that, we can not have dynamic events names like appending the media id to it.

Do you think it would be possible to send the media id as a parameter instead?

This comment has been minimized.

@mzorz

mzorz Jan 7, 2019

Contributor

Do you think it would be possible to send the media id as a parameter instead?

Good call there 👍

This comment has been minimized.

@marecar3

marecar3 Jan 16, 2019

Author Contributor

Hey @etoledom, thanks for the update. I wasn't aware of this one as this approach was working in some lower version. I will fix this. Thanks for letting me know.

cc: @SergioEstevao

}

removeMediaUploadListener( mediaId ) {
gutenbergBridgeEvents.removeListener( 'mediaUpload' + mediaId, this.mediaUpload );

This comment has been minimized.

@etoledom

etoledom Jan 7, 2019

Contributor

I think it would be good to hide the implementation of adding/removing listeners inside the react-native-gutenberg-bridge module. Something similar to what we already have in here.

If we can also hide the RNReactNativeGutenbergBridge.onMediaLibraryPress it would be great too. Even though we didn't do it before, probably is good to have all the available communication methods in one place.

This comment has been minimized.

@marecar3

marecar3 Jan 17, 2019

Author Contributor

Hey @etoledom, thanks for the comments. I have changed the code as we discussed.

@marecar3

This comment has been minimized.

Copy link
Contributor Author

marecar3 commented Jan 14, 2019

Hey @mzorz @etoledom @koke, thanks for the feedback.
I would need a few more hours to hopefully finish hack week after which I will respond to your comments. Thanks again!

marecar3 added some commits Jan 18, 2019

@etoledom

This comment has been minimized.

Copy link
Contributor

etoledom commented Jan 18, 2019

I manage to merge changes from this PR #13096 into my PR and it seems that it's working. Please, take a look at merge result if I have done it in a good way

Looking good! Thank you for that.

marecar3 and others added some commits Jan 18, 2019

RN Mobile - Upload media file ios (#13421)
* Add serverID to finish media upload with success event.

* Refactor code to have early exit.

* Support setting id when selecting from media library.
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…rnmobile/372-add-title-to-gutenberg-mobile

* 'master' of https://github.com/WordPress/gutenberg: (56 commits)
  Save package-lock.json file changes (#13481)
  Plugin: Deprecate gutenberg_add_responsive_body_class (#13461)
  Add speak messages to the feature toggle component. (#13385)
  Plugin: Deprecate gutenberg_kses_allowedtags (#13460)
  Plugin: Deprecate gutenberg_bulk_post_updated_messages (#13472)
  Plugin: Avoid calling deprecated gutenberg_silence_rest_errors (#13446)
  Plugin: Deprecate gutenberg_remove_wpcom_markdown_support (#13473)
  Fix: Categories block: add custom classes only to wrapper (#13439)
  is-shallow-equal: Use ES5 ruleset from eslint-plugin module (#13428)
  Update and Organize Contributors Guide per #12916 (#13352)
  Dismissible-notices: fix text overlapping icon (X) (#13371)
  Framework: Remove 5.0-merged REST API integrations (#13408)
  Plugin: Remove 5.0-merged block registration functions, integrations (#13412)
  Framework: Bump minimum required WP to 5.x (#13370)
  [Mobile] Improve keyboard hide button (#13415)
  Improve castError handling of non strings (#13315)
  Fix: File block add custom class (#13432)
  Consider making Fullscreen Mode effects visible only on larger screens (#13425)
  Update plugin version to 4.9.0 (#13436)
  DateTimePicker: fix prop warning for (#12933)
  ...
Rnmobile/upload captured photo (#13450)
*Upload captured photo
@SergioEstevao
Copy link
Contributor

SergioEstevao left a comment

Looking good!

@youknowriad youknowriad added this to the 5.0 (Gutenberg) milestone Jan 25, 2019

@marecar3 marecar3 referenced this pull request Jan 25, 2019

Merged

Gb/feature upload media file #9120

1 of 1 task complete
@mzorz

mzorz approved these changes Jan 25, 2019

Copy link
Contributor

mzorz left a comment

LGTM! :shipit:

@mzorz

This comment has been minimized.

Copy link
Contributor

mzorz commented Jan 25, 2019

note: I've added small changes in #13516 that would be great to merge into this one first @marecar3 🙇 (thanks!)

mzorz and others added some commits Jan 28, 2019

@marecar3 marecar3 merged commit 2ca9e14 into master Jan 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@marecar3 marecar3 deleted the rnmobile/upload_media_file branch Jan 28, 2019

daniloercoli added a commit that referenced this pull request Jan 28, 2019

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…rnmobile/372-move-title-to-rich-text

* 'master' of https://github.com/WordPress/gutenberg:
  Adds undo/redo support for the title. (#13514)
  Fix typo (#13524)
  RichText: List: Fix indent/outdent (#12667)
  Fixes tense on verb “assign” to correct tense (#13541)
  Plugin: Deprecate metabox supports, fall back to core (#13449)
  Rnmobile/upload media file (#13128)
  Plugin: Remove redundant core compatibility from plugin (#13442)
  Testing: Remove PHPUnit tests covering core functions (#13513)
  Plugin: Deprecate gutenberg_add_gutenberg_post_state (#13471)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment