Fix requestImport call to send the correct arguments. #1494
Conversation
Beside fixing the request for import to send the correct parameters in the callback, we are also improving the interface by replacing a tupple for a structure to make parameters more explicit.
etoledom
left a comment
etoledom
left a comment
There was a problem hiding this comment.
Hey @SergioEstevao - PR looks good!
Added a few code comments, but since this is a hotfix, I don't think any of it should be worked on now.
I did find an odd behavior pasting images from the Photos app:
- If I select a few images on a certain order, after pasting them on Gutenberg, they are added on the reversed order.
I think that if this behavior was the same before, let's keep it like this. If not, it might be good to look into it before merging?
Since this is already better than shipping the issue, we could also decide to fix the reversed order on the next release cycle, that's OK with me too.
Let me know to ✅
| public struct MediaInfo { | ||
| public let id: Int32? | ||
| public let url: String? | ||
| public let type: String? | ||
|
|
||
| public init(id: Int32?, url: String?, type: String?) { | ||
| self.id = id | ||
| self.url = url | ||
| self.type = type | ||
| } | ||
| } | ||
|
|
||
| public typealias MediaPickerDidPickMediaCallback = (_ media: [MediaInfo]?) -> Void |
There was a problem hiding this comment.
Do you think we could avoid some of the optionals?
Not sure if in the callback a nil has a different meaning over an empty array (error maybe?).
And perhaps a MediaInfo should always have a proper type? and/or an id?
Of course we can leave these improvements for another round 👍
There was a problem hiding this comment.
I thought that too, but we will need to check the code on the JS side and see what are the implications of sending nil or not.
Can we do this in the follow up PR?
| let jsFormattedMedia = mediaToReturn.map { mediaInfo in | ||
| return [mediaDictKeys.IDKey: mediaInfo.id as Any, | ||
| mediaDictKeys.URLKey: mediaInfo.url as Any, | ||
| mediaDictKeys.TypeKey: mediaInfo.type as Any] | ||
| } |
There was a problem hiding this comment.
Transforming a MediaInfo instance into the Dictionary for JS might good to have it as part of the MediaInfo struct.
I was thinking about a MediaCollection type to encapsulate this kind of logic, but it's probably too much for a hotfix.
There was a problem hiding this comment.
I followed your suggestion and added a method to do the conversion.
| if allowMultipleSelection { | ||
| callback([jsFormattedMedia]) | ||
| } else { | ||
| callback(jsFormattedMedia) |
There was a problem hiding this comment.
It might be good to merge this and send the full array. If multi select is false, the callback should bring just one media item. Maybe we can enforce that via the callback signature, and keep the bridge simpler 🤔
Again, thinking about the future!
There was a problem hiding this comment.
We don't have control on the callback interface it's defined by RN library.
It's always expecting an array where each item is an argument of the JS function.
What we can do is to cleanup the callback functions on the other side to always expect an array of media.
There was a problem hiding this comment.
Let's do that! On another PR someday in the future 😬
It might be good to create a ticket with all these little cleanup tasks to keep track of them.
|
@etoledom ready for another look. |
etoledom
left a comment
etoledom
left a comment
There was a problem hiding this comment.
Thank you @SergioEstevao !
It's working great now.
Tested Image, Views, Media&Text blocks with all sources and they work perfectly.
Paste images from Photos app also work as expected 🎉
Beside fixing the request for import to send the correct parameters in
the callback, we are also improving the interface by replacing a tupple
for a structure to make parameters more explicit.
This can also be tested on the main app using this PR: wordpress-mobile/WordPress-iOS#12773
To test:
Update release notes:
RELEASE-NOTES.txt.