[RNMobile] Update image size UI control #19232

cameronvoell · 2019-12-19T05:22:01Z

Description

Updates the mobile SelectControl (currently only used on mobile in the Image Block) to use a Cycle Button style UI (CyclePickerCell) instead of nested BottomSheets (PickerCell).

How has this been tested?

See Gutenberg-Mobile PR: wordpress-mobile/gutenberg-mobile#1574
In order to test with image urls updating correctly via api requests, this was tested in WordPress iOS and WordPress Android

Open an article in the editor with an image.
Click the gear button to open image settings
Click the menu row labeled "Size" multiple times.
Note the menu size value and Image within the block are updated simultaneously.

Screenshots

Types of changes

Introduces cycle button UI for updating the size of an image from the BottomSheet menu in an Image Block.
Removes duplicated default image size declarations and instead utilizes image sizes from the editor settings data store.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR. .

test-case-reminder

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.

cameronvoell · 2019-12-19T06:55:33Z

packages/components/src/select-control/index.native.js

@@ -17,7 +17,7 @@ function SelectControl( {
 	const id = `inspector-select-control-${ instanceId }`;

 	return (
-		<PickerCell
+		<CyclePickerCell


Though I did not include it in this PR yet. I am proposing that we remove PickerCell.(though Picker is still super useful!). Though PickerCell gets the BottomSheet nested menu about 80% there, it feels like a fundamental restriction of a react-native-modal Modal nested inside of another, that we will not be able to close the containing Modal before showing the child Modal without editing the underlying library. @Tug @etoledom thoughts?

One reason this might be worth doing now is in order to decouple PickerCell from Cross Platform component refactoring efforts, like seen here in SelectControl.

etoledom

This looks great to me and works perfect 🎉

There's one thing I'm not sure about, maybe @pinarol and @iamthomasbishop, could give more lights:

With this PR we are changing the UX of SelectControl. Probably not a big deal since this is the only place we are using it so far, but for future features this could be not the best approach, if there are a lot more options to choose from.

@iamthomasbishop already gave his 👍 for this UX on this particular use case.

It's also true that the current SelectControl needs a lot of work, so this might be a good enough mid step toward a better component, but I'm unsure of the future plans. Main question is: Is it OK to replace the SelectControl UX like this or would it be better to have a different component with this UX while maintaining the current SelectControl to use in case we need it? (Still considering that PickerCell is not being removed as mentioned here.

I personally think it's fine to merge this PR, but asking here to also give awareness of the change.

etoledom · 2020-01-09T11:46:27Z

packages/block-library/src/image/edit.native.js

-const IMAGE_SIZE_THUMBNAIL = 'thumbnail';
-const IMAGE_SIZE_MEDIUM = 'medium';
-const IMAGE_SIZE_LARGE = 'large';
-const IMAGE_SIZE_FULL_SIZE = 'full';
-const DEFAULT_SIZE_SLUG = IMAGE_SIZE_LARGE;
-const sizeOptionLabels = {
-	[ IMAGE_SIZE_THUMBNAIL ]: __( 'Thumbnail' ),
-	[ IMAGE_SIZE_MEDIUM ]: __( 'Medium' ),
-	[ IMAGE_SIZE_LARGE ]: __( 'Large' ),
-	[ IMAGE_SIZE_FULL_SIZE ]: __( 'Full Size' ),
-};
-const sizeOptions = map( sizeOptionLabels, ( label, option ) => ( { value: option, label } ) );
-


iamthomasbishop · 2020-01-09T16:36:51Z

With this PR we are changing the UX of SelectControl. Probably not a big deal since this is the only place we are using it so far, but for future features this could be not the best approach, if there are a lot more options to choose from.

@etoledom Agreed, and while this is fine for the short-term, I don't even think this is the best solution for this interaction in the long-term. I don't have visibility into whether this will also change/affect other controls, but if so can we document the effects? // cc @cameronvoell

etoledom · 2020-01-10T08:22:13Z

I don't have visibility into whether this will also change/affect other controls, but if so can we document the effects?

This changes SelectControl, instead of presenting a new bottom-sheet with the options to select, it now behaves as shown in the gif. What I mean is that the whole control component behaves this way, not just this instance of it. But again, we don't have other instances in use anywhere else yet.

No other controls are modified at all.

cameronvoell · 2020-01-15T01:13:28Z

This changes SelectControl, instead of presenting a new bottom-sheet with the options to select, it now behaves as shown in the gif. What I mean is that the whole control component behaves this way, not just this instance of it. But again, we don't have other instances in use anywhere else yet.

@etoledom I didn't think we were using SelectControl either, but it turns out it is in the Gallery settings menu and it was added to Video Settings two months ago here:

gutenberg/packages/block-library/src/video/edit-common-settings.js

Lines 58 to 66 in 99d38df

    
           <SelectControl 
        
           	label={ __( 'Preload' ) } 
        
           	value={ preload } 
        
           	onChange={ ( value ) => setAttributes( { preload: value } ) } 
        
           	options={ [ 
        
           		{ value: 'auto', label: __( 'Auto' ) }, 
        
           		{ value: 'metadata', label: __( 'Metadata' ) }, 
        
           		{ value: 'none', label: __( 'None' ) }, 
        
           	] }

I added a new CycleSelectControl (31caac7) to use in Image Settings until we solve the nested bottom sheet menu problem.

etoledom · 2020-01-23T14:06:45Z

I added a new CycleSelectControl (31caac7) to use in Image Settings until we solve the nested bottom sheet menu problem.

Nice addition! Thanks @cameronvoell 🙏

but it turns out it is in the Gallery settings menu and it was added to Video Settings two months ago

For Galleries, I've noticed here that the SelectControl is used also for Image Sizes.

Probably we should sync here and have the same UX in both image sizes and gallery image sizes.

cc @iamthomasbishop @mkevins @pinarol

iamthomasbishop · 2020-01-23T17:01:18Z

Probably we should sync here and have the same UX in both image sizes and gallery image sizes

Agreed, we probably should.

Tug · 2020-01-27T11:10:09Z

packages/block-library/src/image/edit.native.js

 		const { align, url, height, width, alt, href, id, linkTarget, sizeSlug } = attributes;

 		const actions = [ { label: __( 'Clear All Settings' ), onPress: this.onClearSettings } ];

+		const getImageSizeOptions = () => map( imageSizes, ( { label, slug } ) => {


I don't think it's necessary to make that a function here, just computing size options should be fine:

const sizeOptions = imageSizes.map( ( { label, slug } ) => ( { value: slug, label } ) );

Good call, fixed here: 9e71910

Tug · 2020-01-27T11:15:05Z

packages/components/src/mobile/cycle-select-control/index.native.js

+	const id = `inspector-select-control-${ instanceId }`;
+
+	return (
+		<CyclePickerCell


I'm not sure I see the benefit in having both a CycleSelectControl and a CyclePickerCell, they seem mostly identical.

I think I'd prefer to keep it this way for now. It matches other Control / BottomSheet Cell pairs like ToggleControl / SwitchCell and I think there may be implications later on related to cross platform controls, where the control might become independent of where it lives in the UI. This issue has some interesting conversation on that topic #18018

Tug · 2020-01-27T11:20:57Z

packages/components/src/mobile/bottom-sheet/cycle-picker-cell.native.js

+		...cellProps
+	} = props;
+
+	const cycleOptionValue = () => {


A better name for this function imo would be nextOptionValue. Cycle does indicate that the possible options form a cycle but it's hard to understand that this is returning the next one from the currently selected one.
I think readability could be improved here as well, something as simple as this would really help I think

const nextOptionValue = () => { const selectedOptionIndex = findIndex( options, { value } ); return options[ ( selectedOptionIndex + 1 ) % options.length ].value; };

We might want to handle the case where options is en empty array as well ;)

Updated constant name 9aa8aa7 . Added checks here 12ebc85 so the image block will hide image size options if sizes / default are ever empty or not matching, and a check so that the CycleSelectControl value will just be blank if it is ever passed empty options.

Tug · 2020-01-27T11:23:23Z

packages/components/src/mobile/bottom-sheet/cycle-picker-cell.native.js

+		<Cell
+			onPress={ () => onChangeValue( cycleOptionValue() ) }
+			editable={ false }
+			value={ options[ findIndex( options, [ 'value', value ] ) ].label }


Same here, let's create variables with explicit names that should help improve readability.

See comment above, this was improved in 9aa8aa7 and 12ebc85

Tug

Change looks good, I had a few minor comments about the code but otherwise I think it's ready. I like the fact that we create a new SelectControl component for that!

pinarol · 2020-01-27T11:34:00Z

@mkevins this is not urgent but could you open an issue to start a discussion about whether we want to use the same circle button for Gallery or not? I believe we should be consistent among blocks but Gallery settings are x-platform already, so let's see what we can do there. We can dive into this after finishing the upload option.

etoledom · 2020-01-29T10:59:54Z

Thanks @pinarol !
@cameronvoell - Let's land this PR after addressing @Tug's comments 🎉

mkevins · 2020-02-03T04:41:38Z

One consideration I have for this kind of UX is for users low bandwidth / connectivity. In earlier tests with Gallery image size options, I noticed that when I change size options, I must wait for a new file to download (each size option must be cached individually). This mean that to change sizes with a cycle picker, the user may be required to download all sizes to their device, as they cycle through them. I'm not sure how much of an issue this is, or how many users will utilize this feature with low bandwidth. Perhaps not a major issue, just wanted to note that. cc: @iamthomasbishop

cameronvoell · 2020-02-03T09:10:04Z

@cameronvoell - Let's land this PR after addressing @Tug's comments 🎉

Thanks for the review @Tug! Let me know if it looks like I addressed your comments.

Tug

LGTM 👍

iamthomasbishop · 2020-02-10T15:02:58Z

Sorry @mkevins, I totally missed this last week!

One consideration I have for this kind of UX is for users low bandwidth / connectivity. In earlier tests with Gallery image size options, I noticed that when I change size options, I must wait for a new file to download (each size option must be cached individually). This mean that to change sizes with a cycle picker, the user may be required to download all sizes to their device, as they cycle through them. I'm not sure how much of an issue this is, or how many users will utilize this feature with low bandwidth. Perhaps not a major issue, just wanted to note that. cc: @iamthomasbishop

That's a great point, regarding offline or low-bandwidth situations. I wonder if there is any way to load only one size (large or full?) image behind the scenes and have the control only simulate the change? This would bring better performance for any bandwidth scenario.

etoledom · 2020-02-11T16:54:53Z

I wonder if there is any way to load only one size (large or full?) image behind the scenes and have the control only simulate the change?

Sounds like a good option! And I believe it's possible 👍
But I'd add it as an optimisation on a separate ticket.

iamthomasbishop · 2020-02-11T17:34:27Z

Sounds like a good option! And I believe it's possible 👍
But I'd add it as an optimisation on a separate ticket.

Sounds good! 👍

cameronvoell · 2020-02-12T02:54:36Z

packages/block-library/src/image/edit.native.js

@@ -357,9 +357,9 @@ export class ImageEdit extends React.Component {
 			},
 		];

-		const sizeOptions = map( imageSizes, ( { label, slug } ) => ( {
+		const sizeOptions = map( imageSizes, ( { name, slug } ) => ( {


Updated to accommodate this change - #19800

cameronvoell added 2 commits December 18, 2019 21:57

SelectControl on mobile updated to CyclePicker UI

95e7430

Image size options now from editor settings store

c70193b

test-case-reminder bot reviewed Dec 19, 2019

View reviewed changes

Make CyclePickerCell variable name generic

82ab9d3

cameronvoell 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 19, 2019

cameronvoell self-assigned this Dec 19, 2019

cameronvoell marked this pull request as ready for review December 19, 2019 06:41

cameronvoell commented Dec 19, 2019

View reviewed changes

cameronvoell requested a review from etoledom December 19, 2019 07:01

cameronvoell mentioned this pull request Dec 20, 2019

Add image size picker improvements wordpress-mobile/gutenberg-mobile#1574

Closed

1 task

etoledom reviewed Jan 9, 2020

View reviewed changes

New CycleSelectControl added

31caac7

cameronvoell requested review from ajitbohra, chrisvanpatten, jorgefilipecosta, talldan and youknowriad as code owners January 15, 2020 00:40

Merge branch 'master' into rnmobile/fix-image-size-ui-control

c4be661

cameronvoell requested a review from mchowning January 16, 2020 22:19

cameronvoell mentioned this pull request Jan 17, 2020

BottomSheet Nested Options Selector wordpress-mobile/gutenberg-mobile#1780

Open

cameronvoell requested a review from SergioEstevao January 17, 2020 00:48

etoledom self-requested a review January 23, 2020 14:10

Tug reviewed Jan 27, 2020

View reviewed changes

cameronvoell added 2 commits February 2, 2020 22:00

Merge branch 'master' into rnmobile/fix-image-size-ui-control

697ab01

Replaced unneccessary getImageSizeOptions function

9e71910

cameronvoell force-pushed the rnmobile/fix-image-size-ui-control branch from 8bd9ec1 to 9e71910 Compare February 3, 2020 06:32

cameronvoell added 2 commits February 2, 2020 22:39

Adjusted function name and lines for readability

9aa8aa7

Added checks for empty size options

12ebc85

cameronvoell requested a review from Tug February 3, 2020 09:10

Add undefined prop protection

559fb21

Tug approved these changes Feb 10, 2020

View reviewed changes

etoledom mentioned this pull request Feb 11, 2020

Image Sizes behavioral issues wordpress-mobile/gutenberg-mobile#1593

Closed

8 tasks

cameronvoell added 2 commits February 11, 2020 17:36

Merge branch 'master' into rnmobile/fix-image-size-ui-control

d6be598

Incorporating imageSizes defaults change from master

34820b3

cameronvoell commented Feb 12, 2020

View reviewed changes

cameronvoell merged commit 2a57efe into master Feb 12, 2020

cameronvoell deleted the rnmobile/fix-image-size-ui-control branch February 12, 2020 03:47

github-actions bot added this to the Gutenberg 7.6 milestone Feb 12, 2020

mkevins mentioned this pull request Feb 17, 2020

Image size UI control for Gallery block wordpress-mobile/gutenberg-mobile#1921

Closed

cameronvoell mentioned this pull request May 26, 2020

Image Block Size Options - UX Production Ready wordpress-mobile/gutenberg-mobile#1452

Closed

3 tasks

tyxla mentioned this pull request Jun 9, 2022

Lodash: Refactor away from _.findIndex #41635

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] Update image size UI control #19232

[RNMobile] Update image size UI control #19232

cameronvoell commented Dec 19, 2019 •

edited

Loading

test-case-reminder bot left a comment

cameronvoell Dec 19, 2019

etoledom left a comment

etoledom Jan 9, 2020

iamthomasbishop commented Jan 9, 2020

etoledom commented Jan 10, 2020

cameronvoell commented Jan 15, 2020 •

edited

Loading

etoledom commented Jan 23, 2020 •

edited

Loading

iamthomasbishop commented Jan 23, 2020

Tug Jan 27, 2020

cameronvoell Feb 3, 2020 •

edited

Loading

Tug Jan 27, 2020

cameronvoell Feb 3, 2020

Tug Jan 27, 2020 •

edited

Loading

Tug Jan 27, 2020

cameronvoell Feb 3, 2020

Tug Jan 27, 2020

cameronvoell Feb 3, 2020

Tug left a comment

pinarol commented Jan 27, 2020

etoledom commented Jan 29, 2020

mkevins commented Feb 3, 2020 •

edited

Loading

cameronvoell commented Feb 3, 2020

Tug left a comment

iamthomasbishop commented Feb 10, 2020

etoledom commented Feb 11, 2020

iamthomasbishop commented Feb 11, 2020

cameronvoell Feb 12, 2020

[RNMobile] Update image size UI control #19232

[RNMobile] Update image size UI control #19232

Conversation

cameronvoell commented Dec 19, 2019 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

test-case-reminder bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etoledom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamthomasbishop commented Jan 9, 2020

etoledom commented Jan 10, 2020

cameronvoell commented Jan 15, 2020 • edited Loading

etoledom commented Jan 23, 2020 • edited Loading

iamthomasbishop commented Jan 23, 2020

Choose a reason for hiding this comment

cameronvoell Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tug Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tug left a comment

Choose a reason for hiding this comment

pinarol commented Jan 27, 2020

etoledom commented Jan 29, 2020

mkevins commented Feb 3, 2020 • edited Loading

cameronvoell commented Feb 3, 2020

Tug left a comment

Choose a reason for hiding this comment

iamthomasbishop commented Feb 10, 2020

etoledom commented Feb 11, 2020

iamthomasbishop commented Feb 11, 2020

Choose a reason for hiding this comment

cameronvoell commented Dec 19, 2019 •

edited

Loading

cameronvoell commented Jan 15, 2020 •

edited

Loading

etoledom commented Jan 23, 2020 •

edited

Loading

cameronvoell Feb 3, 2020 •

edited

Loading

Tug Jan 27, 2020 •

edited

Loading

mkevins commented Feb 3, 2020 •

edited

Loading