[RNMobile] RangeControl markup by jbinda · Pull Request #18036 · WordPress/gutenberg

jbinda · 2019-10-20T16:12:30Z

Description

PR is connected with #1449 in gutenberg-mobile.

Please also refer to:

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:

Details

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.

…rol-markup

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 · 2019-10-29T03:22:21Z

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

+			minimumValue={ min }
+			maximumValue={ max }
+			value={ value }
+			beforeIcon={ beforeIcon }


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.

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)

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.

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

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).

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.

@pinarol ok so I will:

Document mentioned props in README file

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

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

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

👍

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

Thanks commented

jbinda · 2019-10-29T09:50:44Z

@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 · 2019-11-04T12:02:30Z

packages/components/src/range-control/README.md

+- Type: `String Enum`
+- Values: `none` | `fullWidth` | `topFullWidth`
+- Required: No
+- Platform: Only Mobile


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

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

Current one would be fine.

…rol-markup

jbinda · 2019-11-06T10:53:35Z

@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 · 2019-11-06T13:16:05Z

Great ! Merged 🎉

initial setup

5ff0965

jbinda added the [Status] In Progress Tracking issues with work in progress label Oct 20, 2019

jbinda self-assigned this Oct 20, 2019

jbinda mentioned this pull request Oct 20, 2019

Refactor: BottomSheet.RangeCell markup wordpress-mobile/gutenberg-mobile#1471

Merged

1 task

jbinda added 2 commits October 28, 2019 08:42

adjust passed props

8124817

Merge remote-tracking branch 'origin/master' into rnmobile/range-cont…

03fe42e

…rol-markup

jbinda requested review from mkevins and pinarol October 28, 2019 08:50

jbinda added [Type] Enhancement A suggestion for improvement. and removed [Status] In Progress Tracking issues with work in progress labels Oct 28, 2019

mkevins reviewed Oct 29, 2019

View reviewed changes

jbinda mentioned this pull request Oct 30, 2019

Revisit settings controls API to improve cross platform wordpress-mobile/gutenberg-mobile#1421

Closed

jbinda added 2 commits November 4, 2019 11:20

Merge branch 'master' into rnmobile/range-control-markup

9fc6c6a

update prop description in README.md

5821955

jbinda requested review from ajitbohra, chrisvanpatten, jorgefilipecosta, talldan and youknowriad as code owners November 4, 2019 11:04

pinarol reviewed Nov 4, 2019

View reviewed changes

jbinda added 2 commits November 4, 2019 15:06

update RangeCell change value handler

09c0232

add Platform in prop description

ab013fa

mkevins mentioned this pull request Nov 6, 2019

Mobile gallery draft #18111

Closed

Merge remote-tracking branch 'origin/master' into rnmobile/range-cont…

feaac23

…rol-markup

jbinda requested review from mkevins and pinarol November 6, 2019 11:49

pinarol approved these changes Nov 6, 2019

View reviewed changes

jbinda merged commit 1d8ccc5 into master Nov 6, 2019

jbinda deleted the rnmobile/range-control-markup branch November 6, 2019 13:16

jbinda mentioned this pull request Nov 6, 2019

[RNMobile] RangeCell styling refactor #18157

Merged

5 tasks

youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019

hypest mentioned this pull request Nov 11, 2019

[RNMobile] Native mobile release v1.17.0 #18426

Closed

6 tasks

Conversation

jbinda commented Oct 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

mkevins left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinarol Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbinda commented Oct 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

pinarol left a comment

Choose a reason for hiding this comment

Uh oh!

jbinda commented Nov 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jbinda commented Oct 20, 2019 •

edited

Loading

pinarol Oct 29, 2019 •

edited

Loading

pinarol Nov 4, 2019 •

edited

Loading

pinarol Nov 4, 2019 •

edited

Loading

jbinda commented Nov 6, 2019 •

edited

Loading