[RNMobile] Add mobile Spacer component #17896

lukewalczak · 2019-10-11T08:38:39Z

Description

Ref to gb-mobile: wordpress-mobile/gutenberg-mobile#1436
Fixes: wordpress-mobile/gutenberg-mobile#1428

NOTE: That PR requires RangeControl mobile component -> #17282 to control spacer height using slider.

That PR adds a mobile Spacer component.

UPDATES:

Implemented Accessibility - double press on Cell is focusing the Slider once screen reader is enabled
Corrected dark more check the screenshot
Typing a value works properly check the gif
Styling changes according to @iamthomasbishop comment:
- outline border-width is set to 2px
- outline border color is set to blue-30
- dashed border color is dimmed (30%)
- added padding-bottom to cell (16px)
- added done button

iOS: Screenshot one and two
Android: Screenshot one and two.

Fix focusing slider when VO is ON on iPhoneX
Add an information for VO about current value when changing value

How has this been tested?

Choose Spacer component.
Press settings button
Play with a spacer height using slider:
- default value: 100
- minValue: 20
- maxValue: 500 NOTE: if user sets height 800 on the website and then edits it on the phone, the value from web can be in the middle of the range, so user can decrease and increase the value as well.
Save post on mobile
Open the same post on the web
Play with the height, e.g. set it to 1000 and save a post
Open the same post again on mobile and check the Spacer height

Screenshots

android	ios

setting a spacer height on web

setting a spacer height on mobile

NOTE: On the gifs there is a wrong label Slider instead of Height in pixels which is correct in the code.

Types of changes

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-24T13:32:04Z

@koke I've extracted logic to RangeCell as you requested. I'm still wondering what will be an advantage of moving TextInput into RangeCell instead having it inside a Slider. Could you please elaborate on that?

EDIT: Maybe we don't need a Slider component itself (as a separate component)? - we can have a Slider along with TextInput inside RangeCell. Is that what you meant? - I've refactored it actually in that way, let me know wdyt.

Also after the refactor I was not able to reproduce that 'jiggly' behaviour.

iamthomasbishop · 2019-10-24T15:28:46Z

@lukewalczak Looks nice in the updated screenshots, thanks for wrangling those changes!

dashed border color is dimmed (30%)

Just want to confirm, this is only on dark mode, correct?

added padding-bottom to cell (16px)

This looks better, but can we remove the trailing separator on the last table row?

added done button

This is great! Does this also get an Android equivalent?

Two more things, neither of which are blockers:

If it's not a hassle, can we make the Done text label blue 30 as well?
Regarding the Slider component in general, can we force the Slider/TextInput "group" to a second line in the same table row? This is a bit narrow.

koke · 2019-10-24T16:28:01Z

EDIT: Maybe we don't need a Slider component itself (as a separate component)? - we can have a Slider along with TextInput inside RangeCell. Is that what you meant? - I've refactored it actually in that way, let me know wdyt.

That looks like what I had in mind, but for some reason I thought the Slider part had some more custom behavior than adding the input.

lukewalczak · 2019-10-24T16:33:12Z

@koke Do you think we can accept the structure as it is right now?

lukewalczak · 2019-10-24T18:20:51Z

@iamthomasbishop I've updated PR description and added Android screenshots 🎉

Just want to confirm, this is only on dark mode, correct?

Correct ✅

This looks better, but can we remove the trailing separator on the last table row?

Sure!

Regarding the Slider component in general, can we force the Slider/TextInput "group" to a second line in the same table row? This is a bit narrow

Generally, that issue will be fix within styling issue, but honestly I'm all for doing it within in this PR. Just let me know if you are ok with that ✌️.

iamthomasbishop · 2019-10-24T18:47:33Z

Generally, that issue will be fix within styling issue, but honestly I'm all for doing it within in this PR. Just let me know if you are ok with that ✌️.

@lukewalczak Ah ok, either way is fine by me although the sooner the better 😄

etoledom

Thank you for the improvements @lukewalczak !

I gave it a try with VoiceOver on device (iPhone X), and double-tapping on the cell didn't focus the slider. Did a bit of debugging and I noticed that onCellPress is not being called.
It is called when I tap on it normally, but it won't with VoiceOver.

About: #17896 (comment)

What I wanted to achieve here is to make the parent component (Cell) not accessible, since I want to focus Slider and have also TextInput accessible as well.

That sounds good! It's the approach we follow on other blocks too 👍

Wdyt about having there only one component when screen reader is enabled?

Could you please expand a bit? Do you mean having only slider?
If that's the case, wouldn't auto-focusing the slider be enough and simpler?

Both platforms are reading the slider value in percents, e.g. 100 px is is read as 17 percents.

True that, and it's probably not a good thing in this case. Since 10% can mean different thing on different situations, and in this case, there's no visual help for the user. I think it's best if we can let them know the precise height amount.

A good example is the Music app. It has the the Volume scroller that says its value on percent, and there's the track position scroller. Since songs has different lengths, it won't make much sense to say percents there, so they say the current time together with the total time of the song while dragging it.

If this is blocking other work, I'm happy to ✅ as it is and improve these details separately.

Everything else looks great to me! 🎉
Thank you again!

lukewalczak · 2019-10-28T12:35:31Z

@etoledom Thanks for feedback! I was testing accessibility on iPhone 7, so will dive into the issue on iPhoneX.

A good example is the Music app. It has the the Volume scroller that says its value on percent, and there's the track position scroller. Since songs has different lengths, it won't make much sense to say percents there, so they say the current time together with the total time of the song while dragging it.

I'm not sure what can I do in that matter. Probably I can inform user about current value when user releases the finger / changing a value, however voice over will also pass the info in percents.

lukewalczak · 2019-10-28T14:13:29Z

@etoledom Could you please re-test it? 🙏

etoledom

Accessibility on iOS is working better now. Thanks @lukewalczak for the update!

I added a small code comment, but overall looks good to me 🎉
Great job 🙏

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

…rnmobile/gb-mobile-872-JSApplicationIllegalArgumentException-in-RCTAztecView * 'master' of https://github.com/WordPress/gutenberg: (56 commits) Update: Default gradients. (#18214) Fix: setting a preset color on pullquote default style makes the block invalid (#18194) Fix default featured image size (#15844) Fix postmeta radio regression. (#18183) Components: Switch screen-reader-text to VisuallyHidden (#18165) [rnmobile] Release 1.16.0 to master (#18261) Template Loader: Add theme block template resolution. (#18247) Add a README file for storybook directory (#18245) Add editor-gradient-presets to get_theme_support (#17841) Add "Image Title Attribute" as an editable attribute on the image block (#11070) enables horizontal movers in social blocks (#18234) [RNMobile] Add mobile Spacer component (#17896) Add experimental `ResponsiveBlockControl` component (#16790) Fix mover for floats. (#18230) Rename Component to WPComponent in docstring (#18226) Colors Selector: replace `Aa` text by SVG icon (#18222) Removed gif from README (#18200) makes the submenu items stacked vertically (#18221) Add block navigator to sidebar panel for nav block (#18202) Fix: consecutive updates may trigger a blocks reset (#18219) ...

lukewalczak 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 Oct 11, 2019

lukewalczak self-assigned this Oct 11, 2019

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

This was referenced Oct 11, 2019

Add mobile Spacer component wordpress-mobile/gutenberg-mobile#1436

Merged

Add Spacer block - 1st iteration wordpress-mobile/gutenberg-mobile#1428

Closed

lukewalczak force-pushed the rnmobile/spacer branch from 7d00bf4 to e3f4107 Compare October 17, 2019 11:26

lukewalczak force-pushed the rnmobile/spacer branch from e3f4107 to 54d005d Compare October 17, 2019 11:28

Keep Slider along with TextInput within RangeCell

a1d9a10

lukewalczak force-pushed the rnmobile/spacer branch from 3129e13 to a1d9a10 Compare October 24, 2019 14:40

Small cleanup

833cc76

lukewalczak force-pushed the rnmobile/spacer branch from 6ccd608 to 833cc76 Compare October 24, 2019 15:02

Merge branch 'master' into rnmobile/spacer

103493e

lukewalczak force-pushed the rnmobile/spacer branch from c5c36a7 to afdfb24 Compare October 24, 2019 19:01

Fix missing binds

74d020a

lukewalczak force-pushed the rnmobile/spacer branch from afdfb24 to 74d020a Compare October 24, 2019 19:08

etoledom reviewed Oct 28, 2019

View reviewed changes

lukewalczak added 2 commits October 28, 2019 14:00

Fix focusing slider on iphoneX when VO is on

8c127b9

Adjust a11y voice over

84ade89

Refactor pointerEvents when screen reader is on

1debc8c

lukewalczak force-pushed the rnmobile/spacer branch from 37f83e8 to 1debc8c Compare October 28, 2019 17:15

lukewalczak added 3 commits October 28, 2019 18:25

Announce current value when finished

4a897fd

Improve a11y

7ee822e

Fix a11y on iPhoneX

014cd20

etoledom approved these changes Oct 30, 2019

View reviewed changes

packages/components/src/mobile/bottom-sheet/range-cell.native.js Outdated Show resolved Hide resolved

Update info for translators

2d643c0

pinarol merged commit f73ebe6 into master Nov 1, 2019

etoledom deleted the rnmobile/spacer branch November 6, 2019 16:05

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] Add mobile Spacer component #17896

[RNMobile] Add mobile Spacer component #17896

lukewalczak commented Oct 11, 2019 •

edited

Loading

lukewalczak commented Oct 24, 2019 •

edited

Loading

iamthomasbishop commented Oct 24, 2019

koke commented Oct 24, 2019

lukewalczak commented Oct 24, 2019 •

edited

Loading

lukewalczak commented Oct 24, 2019 •

edited

Loading

iamthomasbishop commented Oct 24, 2019

etoledom left a comment

lukewalczak commented Oct 28, 2019 •

edited

Loading

lukewalczak commented Oct 28, 2019

etoledom left a comment

[RNMobile] Add mobile Spacer component #17896

[RNMobile] Add mobile Spacer component #17896

Conversation

lukewalczak commented Oct 11, 2019 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

lukewalczak commented Oct 24, 2019 • edited Loading

iamthomasbishop commented Oct 24, 2019

koke commented Oct 24, 2019

lukewalczak commented Oct 24, 2019 • edited Loading

lukewalczak commented Oct 24, 2019 • edited Loading

iamthomasbishop commented Oct 24, 2019

etoledom left a comment

Choose a reason for hiding this comment

lukewalczak commented Oct 28, 2019 • edited Loading

lukewalczak commented Oct 28, 2019

etoledom left a comment

Choose a reason for hiding this comment

lukewalczak commented Oct 11, 2019 •

edited

Loading

lukewalczak commented Oct 24, 2019 •

edited

Loading

lukewalczak commented Oct 24, 2019 •

edited

Loading

lukewalczak commented Oct 24, 2019 •

edited

Loading

lukewalczak commented Oct 28, 2019 •

edited

Loading