Skip to content

[RNMobile] Change ripple effect n BottomSheet settings#20922

Merged
jbinda merged 5 commits intomasterfrom
rnmobile/make-range-cell-disabled
Apr 7, 2020
Merged

[RNMobile] Change ripple effect n BottomSheet settings#20922
jbinda merged 5 commits intomasterfrom
rnmobile/make-range-cell-disabled

Conversation

@jbinda
Copy link
Copy Markdown
Contributor

@jbinda jbinda commented Mar 16, 2020 •
edited
Loading

Description

Prevent the Slider in BottomSheet settings to have ripple effect after press on its title.

Please also refer to:
Related gutenberg-mobile PR
Related gutenberg-mobile issue

How has this been tested?

  1. Add Stepper block (it has Slider in settings to control its height)
  2. Open settings to see Slider
  3. Press on Slider title to see if it has ripple effect
  4. Check if you can check Slider value
  • check if a11y works after proposed changes

Screenshots

AFTER:

BEFORE:

Types of changes

Refactor: prevent ripple effect on Slider cell in BottomSheet settings (iOS and Android)
Refactor: disable ripple effect for BottomSheet rows on iOS

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.

@jbinda jbinda added [Type] Bug An existing feature does not function as intended Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Mar 16, 2020
@jbinda jbinda self-assigned this Mar 16, 2020
@jbinda jbinda mentioned this pull request Mar 16, 2020
Merged
2 tasks
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 16, 2020 •
edited
Loading

Size Change: 0 B

Total Size: 889 kB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.4 kB 0 B
build/api-fetch/index.js 3.79 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.03 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 102 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.22 kB 0 B
build/block-library/editor.css 7.22 kB 0 B
build/block-library/index.js 110 kB 0 B
build/block-library/style-rtl.css 7.53 kB 0 B
build/block-library/style.css 7.54 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.5 kB 0 B
build/components/index.js 195 kB 0 B
build/components/style-rtl.css 16.6 kB 0 B
build/components/style.css 16.5 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.7 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.23 kB 0 B
build/date/index.js 5.36 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.05 kB 0 B
build/edit-navigation/index.js 2.71 kB 0 B
build/edit-navigation/style-rtl.css 239 B 0 B
build/edit-navigation/style.css 241 B 0 B
build/edit-post/index.js 92.9 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.1 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/index.js 7.18 kB 0 B
build/edit-widgets/style-rtl.css 3.74 kB 0 B
build/edit-widgets/style.css 3.73 kB 0 B
build/editor/editor-styles-rtl.css 400 B 0 B
build/editor/editor-styles.css 402 B 0 B
build/editor/index.js 42.8 kB 0 B
build/editor/style-rtl.css 3.49 kB 0 B
build/editor/style.css 3.49 kB 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 6.95 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.7 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.84 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.5 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@jbinda
Copy link
Copy Markdown
Contributor Author

jbinda commented Mar 20, 2020 •
edited
Loading

please also refer to this comment for more details

etoledom
etoledom reviewed Mar 23, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @jbinda !

It works as expected with the common flow, but testing with VoiceOver I had troubles. It seems that the slider is not accessible through VoiceOver anymore.

Independent from this PR, I also noticed that when the Spacer Block is selected, there's no feedback from VoiceOver on its body.

@etoledom etoledom mentioned this pull request Mar 24, 2020
Closed
@jbinda
Copy link
Copy Markdown
Contributor Author

jbinda commented Mar 26, 2020 •
edited
Loading

@etoledom @iamthomasbishop

Here is the build with VoiceOver fix on iOS and reverted opacity ripple on Android:
iOS: IPA 25920
Andorid: APK 42612

The cause of broken VoiceOver was passed disable flag to range-cell. I change the opacity with activeOpacity prop and that allow to keep the a11y works and turn-off ripple effect.

I have also disable the ripple for iOS by set activeOpacity to 1 by default for iOS (it is pulled from styles because we have different .scss for each platform).

@iamthomasbishop I wonder if we should change the opacity style for Android in this PR or merge and open anther one, wdyt ?

Independent from this PR, I also noticed that when the Spacer Block is selected, there's no feedback from VoiceOver on its body.

There was some refactor on Spacer here and I didn't check how it works before that. However in my last build it still not working. After we deal with the current PR I think I will raise an issue for that and address in separate PR

etoledom
etoledom approved these changes Mar 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on iOS and Android and its working great!

Feel free to merge after @iamthomasbishop approval.
I'm also happy to test again if we decide to change Android behaviour before merging.

There was some refactor on Spacer here and I didn't check how it works before that. However in my last build it still not working. After we deal with the current PR I think I will raise an issue for that and address in separate PR

Sounds good 👍

@iamthomasbishop
Copy link
Copy Markdown

iamthomasbishop commented Mar 27, 2020

Tested the latest builds on Android + iOS and it feels solid 👍.

Side note: As I mentioned earlier, I would prefer the native ripple effect on Android but this works fine for now, we can always iterate on it.

@jbinda jbinda changed the title [RNMobile] Prevent ripple effect on range cell in BottomSheet settings [RNMobile] Change ripple effect n BottomSheet settings Mar 30, 2020
@jbinda jbinda removed the [Type] Bug An existing feature does not function as intended label Mar 30, 2020
@jbinda
Copy link
Copy Markdown
Contributor Author

jbinda commented Mar 30, 2020

Thanks for checking. I have adjust title of the PR because its more refactor than a BUG specially if we also affect ripple effect for all cells in BottomSheet on iOS

@pinarol wdyt, can we merge this ?

@pinarol
Copy link
Copy Markdown
Contributor

pinarol commented Mar 30, 2020

Yes please, thank you @jbinda !

@jbinda
Copy link
Copy Markdown
Contributor Author

jbinda commented Apr 7, 2020 •
edited
Loading

As spoke with @pinarol on Slack I decided to defer merging this to release 1.25. Release is merged so I also merge.

@jbinda jbinda merged commit 5afac85 into master Apr 7, 2020
@jbinda jbinda deleted the rnmobile/make-range-cell-disabled branch April 7, 2020 06:56
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Apr 7, 2020
@tyxla tyxla mentioned this pull request Apr 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@etoledom etoledom etoledom approved these changes

@pinarol pinarol Awaiting requested review from pinarol

@lukewalczak lukewalczak Awaiting requested review from lukewalczak

Assignees

@jbinda jbinda

Labels

Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)

Projects

None yet

Milestone

Gutenberg 7.9

Development

Successfully merging this pull request may close these issues.

4 participants

@jbinda @iamthomasbishop @pinarol @etoledom