[RNMobile] RangeCell styling refactor #18157

lukewalczak · 2019-10-29T10:15:03Z

Description

NOTE: That PR is based on rnmobile/spacer
Fixes: wordpress-mobile/gutenberg-mobile#1448
Ref to gb-mobile: wordpress-mobile/gutenberg-mobile#1511
Change RangeCell mobile component styling.

How has this been tested?

Open ios or android app
Add Spacer component
Press settings button
Check RangeCell styling

Screenshots

ios dark mode unfocused	ios light mode unfocused

ios dark mode focused	ios light mode focused

android focused	android unfocused

Reset Button functionality

Types of changes

add reset button
move slider to new line
change input styling

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.

lukewalczak · 2019-10-29T10:17:44Z

@iamthomasbishop please look at it 🙏

lukewalczak · 2019-11-05T11:18:15Z

Updated description with a link to gb-mobile PR and resolved conflicts.

jbinda · 2019-11-06T13:30:29Z

I have also resolve conflicts after merge RangeControl markup

etoledom

Thank you for these improvements @lukewalczak - they look great! 🎉

Added a couple of questions (rather than requests) on the code comments.

And I noticed a few small detail testing with bigger text sizes:

If the system font sizes are set to a bigger size (quite common size), the fields start cropping the last digit for numbers bigger than 1000. This can be super confusing.
If the text size is set to the accessibility sizes (the super big ones), the layout start to break. This case is a bit more extreme and we don't really have it well covered in the library in general, so maybe it's not that urgent. But we would want in this case the reset button to be vertically aligned.

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

etoledom · 2019-11-06T15:23:48Z

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

 				accessibilityRole={ 'none' }
 				editable={ false }
 				accessible={ accessible }
 				onPress={ this.onCellPress }
 				accessibilityLabel={ accessibilityLabel }
+				allowReset={ this.handleReset }


It's interesting that the root Cell component provides Reset option, but the reset functionality has to be handled by the parent component. It feels to me more like a generic button that could be anything. Maybe we can add it as an accessory view on this component, similar how we added the Switch?

Just by curiosity, I added allowReset to other controls, it shows the Reset button but it looks a bit off (apart than it does nothing). It doesn't feel to me that it deserves to be on the root Cell component, unless we plan to add reset to more controls? (In which case this might be a good-enough first step). cc @iamthomasbishop

It doesn't feel to me that it deserves to be on the root Cell component, unless we plan to add reset to more controls? (In which case this might be a good-enough first step)

That's correct, I don't think there are any other planned components at the moment that will need an explicit reset button like this.

One more note: if we do end up adding reset to other cells/components, we'll want to display them in the same two-row way which provides more space for the reset button.

Currently we don't have many settings in block but maybe it's not bad idea to allow reset single field (in case we will have much more settings). Correct me if I'm wrong but it seems now it's possible to manually delete/change the value or clear all settings.

Reset button is placed within a Cell component, because we want to locate it next to the label (Height in pixels) which is defined there actually, otherwise it has to be positioned absolutely.

jbinda · 2019-11-06T15:56:15Z

@etoledom thanks for remarks ! We try to discuss big font size issue with Luke tomorrow

lukewalczak · 2019-11-07T14:10:58Z

I did some accessibility tweaks (@etoledom @iamthomasbishop please look at it):

made some space for Reset button, but then label is truncated
TextInput width is multiplied by PixelRatio.getFontScale() therefore we can display 4 digits
by default and when PixelRatio.getFontScale() is lower than 1 TextInput width is 40

default	medium	the largest

pay no attention to the DONE button, something was wrong with a simulator

lukewalczak · 2019-11-07T14:24:04Z

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

+	componentDidMount() {
+		AppState.addEventListener( 'change', this.handleChangePixelRatio );
+	}
+
 	componentWillUnmount() {
 		this.handleToggleFocus();
+		AppState.removeEventListener( 'change', this.handleChangePixelRatio );
+	}
+
+	getFontScale() {
+		return PixelRatio.getFontScale() < 1 ? 1 : PixelRatio.getFontScale();
+	}
+
+	handleChangePixelRatio( nextAppState ) {
+		if ( nextAppState === 'active' ) {
+			this.setState( { fontScale: this.getFontScale() } );
+		}
 	}


Honestly, I think that it's worth to save that info in redux store, because it can be useful in the future to control other components 🤔

It does sound like it would be helpful! But maybe it's too much for this PR.

etoledom

It looks so much better on bigger font sizes! Thanks for the update 🙏

made some space for Reset button, but then label is truncated

That's why is better to modify the layout to vertical when the text sizes are set to accessibility sizes. This is simple to do on iOS but not sure on RN. Anyway, I think it's good as it is for now 👍

TextInput width is multiplied by PixelRatio.getFontScale() therefore we can display 4 digits

Probably, having more than 4 digits is too much of an edge case for this context in particular. I wonder if there are other usages of the Range Control where the numbers could grow a lot bigger.

I think we can revisit if this case becomes a real problem.

From #18157 (comment)

Reset button is placed within a Cell component, because we want to locate it next to the label (Height in pixels) which is defined there actually, otherwise it has to be positioned absolutely.

Makes sense 👍
I'm still not sure about having a specific Reset button that does nothing, and the reset logic has to be implemented on every instance of it. In this case I'd suggest to add it as a generic button (or "box" holder for any component).

But since it's the only component that uses it so far, it might be OK go forward with it and modify it if we need another control with Reset later on. I'm happy with what @pinarol decides.

pinarol · 2019-11-08T10:31:54Z

otherwise it has to be positioned absolutely

I'm still not sure about having a specific Reset button that does nothing, and the reset logic has to be implemented on every instance of it. In this case I'd suggest to add it as a generic button (or "box" holder for any component).

I agree both. I think we can make this better with a simple tweak. Let's not name it as Reset in BottomSheetCell if it doesn't reset anything inside that component. How about just naming it as "customAction" and injecting the title and event handler from outside? Something like this:

ps: Feel free to change namings I don't feel strong about them :)

"box" holder for any component seems too upfront design to me at this point and without knowing the real use cases it can fail to meet future needs.

lukewalczak · 2019-11-08T10:32:24Z

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

+									<View style={ platformStyles.labelIconSeparator } />
+								</View>
+							) }
+							<Text numberOfLines={ 1 } style={ [ defaultLabelStyle, labelStyle ] }>


@etoledom If we remove numberOfLines={ 1 } we can have a not truncated label and it will look like:

wdyt?

I've also checked how it will affect other settings:

before after

Looks better than cropping!
Let's go with it if it does't affect other instances of cells and controls.

I've also checked how it will affect other settings

Again, better than cropping. Looks good to me :)

lukewalczak · 2019-11-08T10:42:18Z

How about just naming it as "customAction" and injecting the title and event handler from outside?

Works for me! Going to adjust it :)

etoledom · 2019-11-08T10:57:33Z

How about just naming it as "customAction" and injecting the title and event handler from outside?

Sounds good! That's what I meant by a add it as a generic button 👍

lukewalczak · 2019-11-08T11:41:44Z

@etoledom @pinarol I've refactored it according to your suggestion and added:

customActionButton={ { title: resetButtonText, handler: this.handleReset } }

pinarol · 2019-11-08T13:17:59Z

Works for me! Also I did a quick testing and it is working pretty smoothly, i am very happy that we got rid of the jumpiness.

etoledom

Looks and work great 🎉
Tested on iOS and Android.

Nice job @lukewalczak !

lukewalczak added 23 commits October 17, 2019 13:40

Revert package-lock.json

5f3a400

Merge branch 'master' into rnmobile/spacer

25832f0

Setting attributes for spacer height

335154d

Correct the condition for setting maximum value

d137d38

Merge branch 'master' into rnmobile/spacer

8d37161

Small code refactor

ae5715f

Improve Accessibility in range-cell

a1cf07b

Merge branch 'master' into rnmobile/spacer

f0b3b28

More accessibility improvements

a807c31

Small code refactor

ca3b8d4

Styling spacer refactor

a390d88

Move logic to RangeCell

90fe027

Keep Slider along with TextInput within RangeCell

a1d9a10

Small cleanup

833cc76

Merge branch 'master' into rnmobile/spacer

103493e

Fix missing binds

74d020a

Fix focusing slider on iphoneX when VO is on

8c127b9

Adjust a11y voice over

84ade89

Refactor RangeCell styles

11e0911

Refactor pointerEvents when screen reader is on

1debc8c

Announce current value when finished

4a897fd

Merge branch 'rnmobile/spacer' into rnmobile/rangecell-styling

80a4714

Small cleanup

047a08e

lukewalczak added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Block] Spacer Affects the Spacer Block labels Oct 29, 2019

lukewalczak mentioned this pull request Oct 29, 2019

RangeCell styling refactor wordpress-mobile/gutenberg-mobile#1511

Merged

1 task

lukewalczak added the Needs Design Feedback Needs general design feedback. label Oct 29, 2019

lukewalczak added 2 commits October 29, 2019 12:13

Improve a11y

7ee822e

Merge branch 'rnmobile/spacer' into rnmobile/rangecell-styling

6d4dde4

Refactor texts localizing

26e7939

lukewalczak removed the [Status] In Progress Tracking issues with work in progress label Nov 5, 2019

merge master

d321afd

etoledom reviewed Nov 6, 2019

View reviewed changes

jbinda and others added 2 commits November 7, 2019 10:45

fix allowReset prop

b9c2b64

Patch allowReset

cf85c25

lukewalczak commented Nov 7, 2019

View reviewed changes

lukewalczak requested a review from etoledom November 7, 2019 16:58

a11y improvements

500d414

lukewalczak force-pushed the rnmobile/rangecell-styling branch from b66c94d to 500d414 Compare November 7, 2019 19:17

etoledom reviewed Nov 8, 2019

View reviewed changes

lukewalczak commented Nov 8, 2019

View reviewed changes

Refactor custom button

5d89738

Remove numberOfLines from cell label

7cd2f4f

mkevins mentioned this pull request Nov 15, 2019

[RNMobile] Gallery - GalleryImage component #18155

Merged

5 tasks

etoledom approved these changes Nov 15, 2019

View reviewed changes

lukewalczak added 2 commits November 15, 2019 13:22

Merge branch 'master' into rnmobile/rangecell-styling

ac07efa

Merge branch 'master' into rnmobile/rangecell-styling

135c4eb

lukewalczak merged commit 2e42f8c into master Nov 15, 2019

lukewalczak deleted the rnmobile/rangecell-styling branch November 15, 2019 17:12

youknowriad added this to the Gutenberg 7.0 milestone Nov 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] RangeCell styling refactor #18157

[RNMobile] RangeCell styling refactor #18157

lukewalczak commented Oct 29, 2019 •

edited

Loading

lukewalczak commented Oct 29, 2019

lukewalczak commented Nov 5, 2019

jbinda commented Nov 6, 2019

etoledom left a comment

etoledom Nov 6, 2019

iamthomasbishop Nov 6, 2019

iamthomasbishop Nov 6, 2019

jbinda Nov 6, 2019 •

edited

Loading

lukewalczak Nov 7, 2019 •

edited

Loading

jbinda commented Nov 6, 2019

lukewalczak commented Nov 7, 2019 •

edited

Loading

lukewalczak Nov 7, 2019

etoledom Nov 8, 2019

etoledom left a comment •

edited

Loading

pinarol commented Nov 8, 2019 •

edited

Loading

lukewalczak Nov 8, 2019 •

edited

Loading

lukewalczak Nov 8, 2019

etoledom Nov 8, 2019

etoledom Nov 9, 2019

lukewalczak commented Nov 8, 2019

etoledom commented Nov 8, 2019

lukewalczak commented Nov 8, 2019

pinarol commented Nov 8, 2019

etoledom left a comment

[RNMobile] RangeCell styling refactor #18157

[RNMobile] RangeCell styling refactor #18157

Conversation

lukewalczak commented Oct 29, 2019 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

lukewalczak commented Oct 29, 2019

lukewalczak commented Nov 5, 2019

jbinda commented Nov 6, 2019

etoledom left a 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

jbinda Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

lukewalczak Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

jbinda commented Nov 6, 2019

lukewalczak commented Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etoledom left a comment • edited Loading

Choose a reason for hiding this comment

pinarol commented Nov 8, 2019 • edited Loading

lukewalczak Nov 8, 2019 • 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

lukewalczak commented Nov 8, 2019

etoledom commented Nov 8, 2019

lukewalczak commented Nov 8, 2019

pinarol commented Nov 8, 2019

etoledom left a comment

Choose a reason for hiding this comment

lukewalczak commented Oct 29, 2019 •

edited

Loading

jbinda Nov 6, 2019 •

edited

Loading

lukewalczak Nov 7, 2019 •

edited

Loading

lukewalczak commented Nov 7, 2019 •

edited

Loading

etoledom left a comment •

edited

Loading

pinarol commented Nov 8, 2019 •

edited

Loading

lukewalczak Nov 8, 2019 •

edited

Loading