[RNMobile] Stepper component by geriux · Pull Request #18864 · WordPress/gutenberg

geriux · 2019-12-02T10:31:24Z

Gutenberg-mobile PR -> wordpress-mobile/gutenberg-mobile#1640

Description

This PR adds a new mobile component Stepper

Note: To be able to test this, I've replaced the SliderControl with this new component StepperControl. I'll revert this before merging.

To test

Open the app
Add the Spacer block
Open its settings
Change the Height using the stepper control
Expect: value should change by pressing in or just with a normal tap.
Accessibility test:
- iOS:
  - Turn on VoiceOver
  - Select the Stepper control
  - Expect: value should change by swiping up / down with one finger. Sound with the value change should play after every change.
- Android:
  - Turn on TalkBack
  - With the settings open: Swipe with one finger from left to right to navigate throughout the UI elements and set focus on the stepper control
  - Once it's focused, use volume keys to change the value
  - Expect: value changing and sound with the change being played.

iOS	Android

Accessibility

For Android devices, users will be able to change the value using the Up / Down volume buttons. It will announce the new value with any change.

For iOS, users will be able to swipe up / down with one finger on the screen to change the value. It will also announce the new value after a change.

Screenshots

iOS

Default	Dark mode

iPad

Android

Types of changes

New feature

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.

geriux · 2019-12-04T17:10:13Z

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

-					<RangeControl
+					<StepperControl
+						icon="screenoptions"
 						label={ __( 'Height in pixels' ) }
-						minimumValue={ minSpacerHeight }
-						maximumValue={ sliderSpacerMaxHeight }
-						separatorType={ 'none' }
+						maxValue={ sliderSpacerMaxHeight }
+						minValue={ minSpacerHeight }
+						onChangeValue={ changeAttribute }
 						value={ height }
-						onChange={ changeAttribute }
-						style={ styles.rangeCellContainer }
+						step={ 20 }


This is for testing only. I will revert this before merging.

etoledom

Great job @geriux ! It works and look great ✨
I really like the accessibility solution, feels so natural 👍

I added a few small code comments, but nothing really important.

There's also one extra detail that I think we could improve. When the cell is pressed, it reacts with opacity. Other cells have some logic to this but this one does nothing:

What do you think about passing a disabled=true property down to the cell's base TouchableOpacity and avoid this opacity animation from happening?

etoledom · 2019-12-10T09:08:11Z

packages/components/src/mobile/bottom-sheet/stepper-cell/README.md

+### separatorType 
+
+Separator type for the `Cell` style.
+
+- Type: `String`
+- Required: No
+- Platform: Mobile


This kind of docs looks great! Thank you for including it.

But this is something I'm not too familiar with, for example:
separatorType is a prop of the base Cell component. As far as I understand, in this file we should document the props that are particular for the stepper component, and inherit properties (even from docs) from the parent component.

Does it makes sense or maybe I'm lost? :)

I think we should add the docs for the StepperControl instead of StepperCell, because StepperControl is the public API. And for inheriting the docs, it can only be possible if parent view is also part of the public @wordpress/components API. Base Cell is not public and i think it won't be public. If we had long term plans for separatorType I'd suggest adding a public parent view but we want to get rid of that as soon as possible so not sure if it's worth it.

I think we should add the docs for the StepperControl instead of StepperCell

That's what I thought at the beginning, but I was surprised that there wasn't docs already for StepperControl .

icon and label are also inherited props. Maybe we want to use BaseControl as an abstraction of our base Cell component (and also extract the TextInput logic from it). I don't mean to do this on this PR though, just ideas about how to improve in the future.

Thanks for the feedback! It makes sense to move the docs to StepperControl and remove the props that are not specific for the component itself. I'll start with that but it's also good to know how we could improve it in the future.

etoledom · 2019-12-10T09:09:51Z

packages/components/src/mobile/bottom-sheet/stepper-cell/index.native.js

+				onAccessibilityAction={ ( event ) => {
+					switch ( event.nativeEvent.actionName ) {
+						case 'increment':
+							this.onIncrementValue();
+							break;
+						case 'decrement':
+							this.onDecrementValue();
+							break;
+					}
+				} }>


Great solution for accessibility, works like a charm ✨

etoledom · 2019-12-10T09:25:05Z

packages/components/src/mobile/bottom-sheet/stepper-cell/index.native.js

+			/* translators: accessibility text. Inform about current value. %1$s: Control label %2$s: Current value. */
+			_x( '%1$s. Current value is %2$s', 'Increase or decrement to change the value' ),


Not sure if we should use _x here.

As far as I understand, _x is used when the same frase might need different translations depending on the context. We do have another instance of this same string, but not sure if the context is different (this is a picker while the other is the slider).

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

Line 152 in 2ebfb64

_x( '%1$s. Current value is %2$s', 'Slider for picking a number inside a range' ),

If we keep it, I'd suggest to use a similar context string, something like: Picker for selecting a number inside a range.

Oh right! we do have the Slider one. I'll change the _x thanks!!

etoledom · 2019-12-10T09:25:13Z

packages/components/src/mobile/bottom-sheet/stepper-cell/index.native.js

+				accessibilityActions={ [
+					{ name: 'increment', label: 'increment' },
+					{ name: 'decrement', label: 'decrement' },
+				] }


From what I see here, labels for standard actions (like increment/decrement) are not required, but if we add them, the best would be to localise them. I'd suggest to not add them in this case and let the system do their magic.

Sounds good! Way better to avoid handling those localizations =D

etoledom · 2019-12-10T09:49:33Z

docs/manifest-devhub.json

+	{
+		"title": "StepperCell",
+		"slug": "stepper-cell",
+		"markdown_source": "../packages/components/src/mobile/bottom-sheet/stepper-cell/README.md",
+		"parent": "components"
+	},


Not sure if we should add this here 🤔
StepperCell is an internal component that we make available via the native version of StepperControl.

@gziolo any light on this?

It's interesting that it gets detected. I guess this is how it works with README.md files added to @wordpress/components.

I don't think it's internal as it gets exposed and consumed in the block library. We should just make it clear that it will work only with the React Native code. @youknowriad - any thoughts on how to make it possible to ignore all mobile docs from being exposed in the documentation published at https://developers.wordpress.org?

I don't think it's internal as it gets exposed and consumed in the block library.

We are currently using the controls abstractions (StepperControl, RangeControl, etc...), so the mobile-only -Cell components are not being consumed by any other libraries (not directly).

We are planning on adding these docs directly to StepperControl instead. Wdyt?

etoledom · 2019-12-10T09:57:09Z

One more thing! 😁

Tested a bigger text size on Android and the layout broke:

I wouldn't worry about the huge iOS Accessibility font sizes on this iteration (since it probably will be quite troublesome to fix) but this font sizes (just a bit bigger) are quite common. It would be great to check this out.

geriux · 2019-12-11T11:21:14Z

Great job @geriux ! It works and look great ✨
I really like the accessibility solution, feels so natural 👍

Thank you! I think so too! Tested different methods and this one seemed just right 😁

What do you think about passing a disabled=true property down to the cell's base TouchableOpacity and avoid this opacity animation from happening?

Good idea! I'll check it out 😃

geriux · 2019-12-11T16:46:42Z

@etoledom Updated with some changes =)

etoledom

Thank you for the update @geriux - It's working great now 🎉

Let's wait for @gziolo and/or @youknowriad to ✅ the documentation bit.
The rest looks good to me. Great job!

geriux · 2019-12-12T10:26:41Z

Thank you for the update @geriux - It's working great now 🎉

Let's wait for @gziolo and/or @youknowriad to ✅ the documentation bit.
The rest looks good to me. Great job!

Yay! Sounds good! Thank you! 🎉

gziolo · 2019-12-12T10:53:30Z

I have two very basic questions to better understand the purpose of this new component.

What's the difference between this component and RangeControl?

See:

Why is it specific to mobile only?

pinarol · 2019-12-12T11:07:39Z

@gziolo Basically we want the Stepper component to represent controls that has ≤ 6 or 8 options instead of Slider on mobile. It is a UX request made by @iamthomasbishop so I'll defer to him to provide better insight about this.

iamthomasbishop · 2019-12-12T17:41:18Z

It is a UX request made by @iamthomasbishop so I'll defer to him to provide better insight about this.

Yes, as @pinarol mentioned Stepper (or even Picker, which @koke has mentioned) is better suited than a Slider for controls with a small range of options. For example, Apple's HIG suggests the following:

Steppers work well for making small changes that require a few taps.

…/stepper-control

gziolo · 2019-12-19T11:49:07Z

I opened #19236 to ensure that mobile-only components aren't exposed in WordPress developers' docs. It should make it easier for you to keep all the work documented.

Gerardo Pacheco added 3 commits November 27, 2019 08:30

Stepper Control

1fa0c72

Stepper component

2fc03f0

Update docs

76704e5

geriux changed the title ~~[RNMobile] Stepper control~~ [RNMobile] Stepper component Dec 2, 2019

geriux mentioned this pull request Dec 2, 2019

Stepper component wordpress-mobile/gutenberg-mobile#1640

Merged

1 task

RNMobile - Switch RangeControl with StepperControl in Spacer Block

5541f3f

geriux commented Dec 4, 2019

View reviewed changes

geriux 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 5, 2019

geriux marked this pull request as ready for review December 5, 2019 10:03

geriux requested review from ajitbohra, chrisvanpatten, jorgefilipecosta, talldan and youknowriad as code owners December 5, 2019 10:03

geriux requested a review from etoledom December 5, 2019 16:54

etoledom reviewed Dec 10, 2019

View reviewed changes

RNMobile - StepperControl accessibility changes and README file

70c35c2

geriux requested a review from etoledom December 11, 2019 16:46

etoledom approved these changes Dec 12, 2019

View reviewed changes

etoledom requested a review from gziolo December 12, 2019 10:19

gziolo added the [Package] Components /packages/components label Dec 12, 2019

Gerardo Pacheco added 2 commits December 18, 2019 09:27

[RNMobile] - Restore RangeControl in the Spacer block

b978650

Merge branch 'master' of github.com:WordPress/gutenberg into rnmobile…

8159ab4

…/stepper-control

geriux merged commit 619b80d into master Dec 18, 2019

geriux deleted the rnmobile/stepper-control branch December 18, 2019 09:10

gziolo mentioned this pull request Dec 19, 2019

Documentation: Do not expose documentation for mobily only components #19236

Merged

youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020

geriux mentioned this pull request Jan 29, 2020

[RNMobile] - Replace Slider with Stepper for columns settings on Gallery block #19947

Merged

6 tasks

geriux mentioned this pull request Feb 14, 2020

[RNMobile] - Gallery Block: Columns Control #20231

Merged

6 tasks

		/* translators: accessibility text. Inform about current value. %1$s: Control label %2$s: Current value. */
		_x( '%1$s. Current value is %2$s', 'Increase or decrement to change the value' ),

Conversation

geriux commented Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

To test

Screenshots

iOS

Android

Types of changes

Checklist:

Uh oh!

geriux Dec 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!

etoledom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinarol Dec 10, 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!

etoledom Dec 11, 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!

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!

etoledom Dec 11, 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!

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

Uh oh!

Uh oh!

geriux commented Dec 11, 2019

Uh oh!

geriux commented Dec 11, 2019

Uh oh!

etoledom left a comment

Choose a reason for hiding this comment

Uh oh!

geriux commented Dec 12, 2019

Uh oh!

gziolo commented Dec 12, 2019

Uh oh!

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

Uh oh!

Uh oh!

iamthomasbishop commented Dec 12, 2019

Uh oh!

gziolo commented Dec 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

geriux commented Dec 2, 2019 •

edited

Loading

geriux Dec 4, 2019 •

edited

Loading

pinarol Dec 10, 2019 •

edited

Loading

etoledom Dec 11, 2019 •

edited

Loading

etoledom Dec 11, 2019 •

edited

Loading

etoledom commented Dec 10, 2019 •

edited

Loading

pinarol commented Dec 12, 2019 •

edited

Loading