[try] Image Size with networking on native client by etoledom · Pull Request #1455 · wordpress-mobile/gutenberg-mobile

etoledom · 2019-10-17T07:29:53Z

Alternative to #1453

This PR pass the networking request intent to the client app, and awaits for a response.

As a proof of concept, this PR implements the minimum necessary to make Image Size works.
In the future we might need to pass the request method and parameters.

Related PRs:

gutenberg: [RNMobile] Implement getMedia on Image block. WordPress/gutenberg#17979 (same as [try] Image Size networking #1453)
WPiOS: [Gutenberg] Image Size networking (native) WordPress-iOS#12714
WPAndroid: Implement React Native bridge method for making api requests WordPress-Android#10779
FluxC: merged

To test on iOS refer to WPiOS PR: wordpress-mobile/WordPress-iOS#12714
To test on Android refer to WPAndroid PR: wordpress-mobile/WordPress-Android#10779

Update release notes:

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

siteSlug and extraHTTPHeaders were added to be used on apiFetch

etoledom · 2019-11-07T07:59:40Z

@mchowning FYI - Just rebased all relevant branches to keep them up to date.

…working-native

koke · 2019-11-08T11:52:42Z

src/api-fetch-setup.js

+		return response;
+	};
+
+	return responsePromise.then( parseResponse ).catch( ( error ) => {


Do we need the then/error if they're just returning the passed value?

I've updated parseResponse so it no longer just returns the passed value like it used to. I made this change because I have Android returning a json string because that's much simpler to do. If there's an issue with that I can certainly update it to return a json object like iOS.

With respect to the catch clause, I believe adding that does change the behavior so that now the resulting promise essentially "resolves" with the error (i.e., the resulting promise will pass the error in any subsequent then calls). Seems like this probably is not desired behavior. What do you think @etoledom ?

…e-networking-native

mchowning · 2019-11-11T21:50:43Z

Hey @etoledom ! I merged the code from release/1.17 into this branch and updated the PR to target that release branch.

Helps to avoid getting unexpected responses and hard to track down bugs: wordpress-mobile/WordPress-iOS#12714 (comment)

…e-networking-native

etoledom · 2019-11-15T17:22:51Z

Overall it looks good to me ✅

I have found a few issues that I think are beyond this (Networking specific) PR:

[WPiOS]: When selecting different a different image size, the image's size doesn't change visually. This is due to the Image.getSize call fails to return the correct size on urls with the form ...?w=100 (from what I tested), returning the full size all the time. Then this value is sent to the native side which uses Photon API to get the image with the given size.
[WPiOS (Probably Android too)] When the image size request fails, Gutenberg won't ever request it again for the same image (on the same session). This might be a problem for users with bad connectivity.
Given that the previous problem occurs for any reason (like no internet connection or a self-hosted endpoint failing), setting the image size will still succeed on updating the slug, but it will silently fail to update the image url to the proper one of the smaller size. I believe we could not show the Image Size picker option if we don't have the Images size information requested.
The default Image Size slug is Large, but the actual image URL is the full size one. On web, a new added image has set the proper Large image URL.
When an image has a slug set (let's say Thumb) and we change the image for a new one, the ImageSlug is not updated to the default one (it will remain as Thumb on this example).

SergioEstevao · 2019-11-18T13:18:07Z

src/test/api-fetch-setup.test.js

+	'wp/v1/media/', // made up example
+];
+
+describe( 'isPathSupported', () => {


Love the tests for this.

SergioEstevao

Tested this on iOS and it's working great.

I tested with WP.com normal, Atomic WP.com and self-hosted sites.

Love the use of Result and Promise in Swift and JS.

Great work.

…working-native

etoledom added [Type] Enhancement Improves a current area of the editor Media labels Oct 17, 2019

etoledom added this to the 1.16 milestone Oct 17, 2019

etoledom self-assigned this Oct 17, 2019

etoledom mentioned this pull request Oct 17, 2019

[try] Image Size networking #1453

Closed

1 task

hypest modified the milestones: 1.16, 1.17 Nov 1, 2019

etoledom added 9 commits November 7, 2019 08:16

Implement api-fetch middlewares

4a564b9

Add new init properties to iOS Bridge

7830e20

siteSlug and extraHTTPHeaders were added to be used on apiFetch

Update gutenberg ref

4e9aa38

Add defaults to example app

8d81114

Remove the need for a custom fetchHandler

7ea196a

Use bridge to send a fetch request to client app

1797382

Modify bridge to handle fetch request

e366d01

Update example app

e3b8e72

Remove not-needed function name

e675c61

etoledom force-pushed the try/image-size-networking-native branch from 4320681 to e675c61 Compare November 7, 2019 07:57

etoledom force-pushed the try/image-size-networking-native branch from 9a751e0 to e675c61 Compare November 8, 2019 08:11

Update gutenberg ref

d9c910b

etoledom mentioned this pull request Nov 8, 2019

[Gutenberg] Image Size networking (native) wordpress-mobile/WordPress-iOS#12714

Merged

1 task

Merge remote-tracking branch 'origin/develop' into try/image-size-net…

97c47ac

…working-native

etoledom mentioned this pull request Nov 8, 2019

[RNMobile] Implement getMedia on Image block. WordPress/gutenberg#17979

Merged

koke reviewed Nov 8, 2019

View reviewed changes

mchowning added 2 commits November 11, 2019 15:12

Add Android native handling of api calls

a353978

Merge remote-tracking branch 'origin/release/1.17' into try/image-siz…

6c9e719

…e-networking-native

mchowning changed the base branch from develop to release/1.17 November 11, 2019 21:50

mchowning mentioned this pull request Nov 11, 2019

Implement React Native bridge method for making api requests wordpress-mobile/WordPress-Android#10779

Merged

4 tasks

mchowning requested a review from cameronvoell November 11, 2019 22:00

mchowning marked this pull request as ready for review November 11, 2019 22:01

mchowning requested review from SergioEstevao and marecar3 November 11, 2019 22:01

etoledom and others added 8 commits November 15, 2019 15:14

[iOS Bridge] Change on fetch completion handler name

e6c77ee

[iOS Bridge] Adding code comments on gutenbergDidRequestFetch

0ac4672

Update iOS example app with changes on bridge

8bf3535

Have fetch handler immediately reject unsupported paths

a4aaac9

Helps to avoid getting unexpected responses and hard to track down bugs: wordpress-mobile/WordPress-iOS#12714 (comment)

Fix test

ea9b5d5

Merge remote-tracking branch 'origin/release/1.17' into try/image-siz…

c85adc7

…e-networking-native

Adding resolver to apiFetch error case

ae30ed6

Fix lint error

52c89f1

SergioEstevao reviewed Nov 18, 2019

View reviewed changes

SergioEstevao approved these changes Nov 18, 2019

View reviewed changes

etoledom modified the milestones: 1.17, 1.18 Nov 19, 2019

etoledom added 2 commits November 19, 2019 07:25

Update gutenberg ref

e44264c

Merge remote-tracking branch 'origin/develop' into try/image-size-net…

9183a39

…working-native

etoledom changed the base branch from release/1.17 to develop November 19, 2019 06:29

etoledom added 2 commits November 20, 2019 09:25

Update gutenberg ref

7082256

Update gutenberg ref

6090fd9

etoledom merged commit 96d6f5d into develop Nov 20, 2019

etoledom deleted the try/image-size-networking-native branch November 20, 2019 09:16

maxme mentioned this pull request Nov 29, 2019

Release v1.18.0 #1627

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[try] Image Size with networking on native client#1455

[try] Image Size with networking on native client#1455
etoledom merged 25 commits intodevelopfrom
try/image-size-networking-native

etoledom commented Oct 17, 2019 •

edited by mchowning

Loading

Uh oh!

etoledom commented Nov 7, 2019

Uh oh!

koke Nov 8, 2019

Uh oh!

mchowning Nov 13, 2019

Uh oh!

mchowning commented Nov 11, 2019

Uh oh!

etoledom commented Nov 15, 2019 •

edited

Loading

Uh oh!

SergioEstevao Nov 18, 2019

Uh oh!

SergioEstevao left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

etoledom commented Oct 17, 2019 • edited by mchowning Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etoledom commented Nov 7, 2019

Uh oh!

koke Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

mchowning Nov 13, 2019

Choose a reason for hiding this comment

Uh oh!

mchowning commented Nov 11, 2019

Uh oh!

etoledom commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergioEstevao Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

SergioEstevao left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

etoledom commented Oct 17, 2019 •

edited by mchowning

Loading

etoledom commented Nov 15, 2019 •

edited

Loading