Skip to content

[RNMobile]: Image block: Hide Size options when Image object is undefined.#19037

Merged
etoledom merged 1 commit intomasterfrom
rnmobile/hide-image-size-option-conditionally
Dec 16, 2019
Merged

[RNMobile]: Image block: Hide Size options when Image object is undefined.#19037
etoledom merged 1 commit intomasterfrom
rnmobile/hide-image-size-option-conditionally

Conversation

@etoledom
Copy link
Copy Markdown
Contributor

@etoledom etoledom commented Dec 10, 2019

This PR fixes point 3 of wordpress-mobile/gutenberg-mobile#1593

gutenberg-mobile side PR: wordpress-mobile/gutenberg-mobile#1669

When the image object is not present, the Image Size option won't be present in the Image Options.

The Image object is fetched on a network call for images stored in the WP Media Library. For images added via link, this object is never fetched.

Other cause of the object being missing is network call errors.

With this change we are looking to avoid the case when the ImageSize is displayed in the UI, but it does nothing.

To Test:

  • Create a new post on Gutenberg Web.
  • Add an image via link.
  • Add a second image from WP Media Library.
  • Save the post and open it in WPiOS/WPAndroid Gutenberg Mobile (running this branch in metro)
  • Check that the Image Sizes options are displaying on the image from the Media Library.
  • Check that the Image Size options are NOT displaying in the image added from external link.

This avoids showing the option when it won't do anything.
@etoledom etoledom added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Dec 10, 2019
@etoledom etoledom requested a review from mchowning December 10, 2019 14:43
@etoledom etoledom self-assigned this Dec 10, 2019
Copy link
Copy Markdown

@test-case-reminder test-case-reminder bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some suggested test cases for this PR.

WordPress iOS:

  • Simultaneous uploads - steps
  • Image block - Insert image from device (failing) - steps
  • Image block - Insert image from device (cancel) - steps
  • Image block - Add Caption - steps
  • Image block - Close/Re-open post with an ongoing image upload - steps
  • Image block - Close post with an ongoing image upload - steps
  • Image block - Switch to classic editor with an image block in page - steps

WordPress Android:

  • Simultaneous uploads - steps
  • Image block - Insert image from device (failing) - steps
  • Image block - Insert image from device (cancel) - steps
  • Image block - Add Caption - steps
  • Image block - Close/Re-open post with an ongoing image upload - steps
  • Image block - Close post with an ongoing image upload - steps
  • Image block - Switch to classic editor with an image block in page - steps

If you think that suggestions should be improved please edit the configuration file here. You can also modify/add test-suites to be used in the configuration.

If you are a beginner in mobile platforms follow build instructions.

onChange={ this.onSetNewTab }
/>
{ // eslint-disable-next-line no-undef
__DEV__ &&
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it seems those image added via url don't have an id property, maybe it would be clearer to use id as a condition instead since it refers to an id inside the WordPress Media library,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the ideal scenario, but we can also get networking errors fetching the image object. Specially those mystery 401 errors that sometimes we get on specific images on self-hosted.

There are some experiments to have (some kind of) authentication on self hosted: wordpress-mobile/WordPressKit-iOS#202 , until then I'd prefer to go with checking if the object is present.

Copy link
Copy Markdown
Contributor

@Tug Tug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Product reviewed on Android. This LGTM 👍

@etoledom etoledom merged commit da83121 into master Dec 16, 2019
@etoledom etoledom deleted the rnmobile/hide-image-size-option-conditionally branch December 16, 2019 12:35
@etoledom
Copy link
Copy Markdown
Contributor Author

Thank you!

@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants