[RNMobile] RangeControl markup

[jbinda]

Description

PR is connected with #1449 in gutenberg-mobile.

Please also refer to:

Related gutenberg-mobile PR

It presents ability to select all parents in cascade way (according to the way how selecting in groups works on web version)

How has this been tested?

• Add below range cell implementation inside InspectorControls in edit.native.js

Usage of RangeControl

<RangeControl
icon={ 'admin-settings' }
label={ __( 'Slider' ) }
value={ sliderVal }
initialPosition={ 25 }
min={ 20 }
max={ 100 }
separatorType={ 'fullWidth' }
onChange={ this.updateSlider }
/>

• add handler to change slider value

Handler to change slider value

updateSlider( newVal ) {
    this.props.setAttributes( { sliderVal: newVal } );
}

• reset sliderVal to null in onClearSettings() function

I have also test this feature by merging this PR changes and this branch into Gallery try feature.

After switch to Gallery branch I need to temporary comment some code in edit.js in block-library/gallery which produced error. See details below:

componentDidMount() {
	const { attributes, mediaUpload } = this.props;
	const { images } = attributes;
	if ( every( images, ( { url } ) => isBlobURL( url ) ) ) {
		const filesList = map( images, ( { url } ) => getBlobByURL( url ) );
		forEach( images, ( { url } ) => revokeBlobURL( url ) );
		// mediaUpload( {
		// 	filesList,
		// 	onFileChange: this.onSelectImages,
		// 	allowedTypes: [ 'image' ],
		// } );
	}
}

Then I was able to test if passing mobile specific props do not breaks anything in web and if web props do not break anything in mobile.

Screenshots

Types of changes

Adds new layer on top of BottomSheet.RangeCell to use it in the same way as Sidebar RangeContol slider in web version.

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.

[mkevins]

One place where the behavior for RangeControl on mobile deviates from web is that the effects are immediate for web: https://github.com/WordPress/gutenberg/tree/03fe42e31bc3da8d2852dd6203219c1c97068cd9/packages/components/src/range-control#immediate-effects. I believe we introduced some changes to RangeCell to work around a "jumpiness" bug, which may be what is preventing the values from updating immediately in RangeControl (i.e. the mobile slider's onChangeValue only fires after the react-native slider's onSlidingComplete is invoked).

Is there another way to address the "jumpiness" in the underlying slider implementation which preserves the "immediate effects" described in the web implementation for RangeControl?

[mkevins]

It seems that a few of these props (e.g. beforeIcon, afterIcon, etc.) are not used in RangeCell, and are thus passed along to the Cell wrapping them:

gutenberg/packages/components/src/mobile/bottom-sheet/range-cell.native.js

Line 24 in 03fe42e

...cellProps

. Cell uses a prop called icon, but neither beforeIcon, nor afterIcon are supported there.

In the current draft PR for gallery, I'm using icon, but this won't work for web, since the props not used in the web implementation of RangeControl will be passed on to the input element: https://github.com/WordPress/gutenberg/tree/03fe42e31bc3da8d2852dd6203219c1c97068cd9/packages/components/src/range-control#props. (i.e. <input icon="some-icon">).

Note: I tried omitting icon, but this seems to break the layout of the slider. I believe that is an issue with the RangeCell mobile component, and not this PR.

[jbinda]

It's ok when you are using separate edit.js and edit.native.js. Then you can pass different set of props and the problem is gone. I assume you are trying to use one common edit.js file both for web and mobile ? I wonder if it's the only prop that cause problems. I guess not ( I believe we are able to find more props e.g. separatorType={ 'fullWidth' } can be passed to mobile but I'm afraid it's not desirable to obtain it on the web)

[pinarol]

In the current draft PR for gallery, I'm using icon, but this won't work for web

What’s that icon for? Does it have the same purpose with either beforeIcon or afterIcon? @mkevins

@jbinda right we have 1 edit.js. And we want to make components library cross platform, thus, we should be able to use same props for both platforms. Ofc there can be platform specific props but it should be necessary only when the actual behavior behimd that prop is different and platform specific. I was wondering, Is it possible to hide separatorType inside RangeControl native.js? If we’ll use the same separator type for every case I think we can do that.

[pinarol]

Btw, we have a tech debt to get rid of separatorType somehow.

[mkevins]

Hi @pinarol 👋

What’s that icon for? Does it have the same purpose with either beforeIcon or afterIcon?

That icon prop is used by Cell, and it seems to be similar to beforeIcon. RangeControl does not have this prop, though. I'm currently using it in the draft PR to prevent the slider layout from breaking (in the BottomSheet).

[pinarol]

That's the plan for separatorType, but we couldn't get to start working on that yet. And we don't need to do this as a part of this PR.

For these ones:

accessibilityLabel,
accessibilityHint,
accessibilityRole,

I'd check if they crash web and make a fix accordingly but I wouldn't necessarily document them in README as they are available for all react-native components in general.

[jbinda]

@pinarol ok so I will:

1. Document mentioned props in README file
2. Make sure passing extra props specific to one platform do not break anything in other platform

Is there anything else that we should address in this PR ?

I also saw that Spacer feature was merged so I hope shortly we will also merge latest changes related to RangeCell made by Luke which was targeting Spacer branch

[pinarol]

1. Document mentioned props in README file

Yes, and it'd be enough if we only document icon, separatorType as new props. But we should be adding proper Platform attributes for all props as we did on ..../media-placeholder/README.md

2. Make sure passing extra props specific to one platform do not break anything in other platform

👍

Is there anything else that we should address in this PR ?

Not anything else that I can think of.

I also saw that Spacer feature was merged so I hope shortly we will also merge latest changes related to RangeCell made by Luke which was targeting Spacer branch

👍

[jbinda]

I have update README (can you take a look if the description make sense ?), CI passed after sync latest gutenberg/gutenberg-mobile repo changes.

Now I'm testing if passing platform specific props do not break mobile/web app

[pinarol]

Thanks commented

[jbinda]

@mkevins thanks for comments.

You're right. I have noticed that calling setAttributes method to change the value pin to slider continuously when moving slider is causing the "jumpiness" bug. Trigger state change after slide complete event helps to solve it. Maybe we need more investigation on it to see if there is another way to deal with both things.

However we still working on styling with @lukewalczak so we should be able to cover the beforeIcon and afterIcon as well as broken layout when you did not pass icon prop to slider.

[pinarol]

This would be better:

Platform: Mobile

Let's also add Platform attribute to other props similar to what we did in media-placeholder/README.md
And add these where suitable:

Platform: Web | Mobile

Platform: Web

[jbinda]

Ok. You would like me to do this in current PR or open new one ?

[pinarol]

Current one would be fine.

[jbinda]

@pinarol I have update README.md and update PR description on how I have been testing it according to passing extra props

I have used @mkevins gallery try branch and also @lukewalczak RangeCell styling. I think at this moment it is ready to ship.

I have also noticed this but I think at this moment we're fine and can refactor with separate PR after we get final conclusion from this thread.

[pinarol]

I think this is looking/working good 👍

Mobile part is tested with the code snippet provided

Web part is tested by passing some mobile specific props to RangeControl and verifying that they don't break anything.

[jbinda]

Great ! Merged 🎉