Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Rnmobile/upload media file #13128
Changes from 7 commits
49bb394
96a73c5
93057b2
0a0764e
1876d22
8a363f9
db23d43
ff11770
704b755
e2b08e1
40b7c9c
d5910a8
08108a0
047f1d8
4f55ba9
7b823ec
e5d8d3f
2668862
408cb1e
50c9a12
d840e30
5f2889f
21920fd
4f4d5f6
876b445
d436e9b
4c27638
0ab5a1b
1f9c722
e54d5b9
e2cb5c9
046213a
7f05b9c
b94bede
77a1fbe
8264c52
c2a8cf5
2e7267b
08f5cc2
9b2f540
f3b9607
5814a4c
e0d5d9b
a5a200b
c9d7b9f
9ea8f39
b822344
e4bf38e
6129033
b5e679b
74aeb23
345d356
9fecd69
028ab55
93e6e95
2fc48c9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call there 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @etoledom, thanks for the comments. I have changed the code as we discussed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason for having a double underscore "__" in this name? should it be a normal dash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is part of the CSS coding standards for Gutenberg. I would ask if we actually need the
className
in the native implementationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thanks @koke. We don't need that one, I removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL - thanks @koke
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. Suchconst http
constant should belong within the scope of such function only, etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See for reference https://github.com/wordpress-mobile/WordPress-Android/pull/8879/files#diff-02f567e707c1df878dfea548cfec3da7R453, this:
as in Android's utils https://developer.android.com/reference/android/webkit/URLUtil#isNetworkUrl(java.lang.String)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can
import { isURL } from '@wordpress/url'
😉https://github.com/WordPress/gutenberg/blob/master/packages/url/README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mzorz , @koke . I have updated the code to use isURL method like @koke suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see this value was there before this change, but wondering why
height: 200
, is there anything specific to that value?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can ask @etoledom about this one, as this change was made a long time ago?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that this was changed with this PR #13096
so @etoledom , you can ignore above question :) Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be passed through i18n, like
_( 'Write caption...' )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mzorz ! Fixed and pushed.