Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RNMobile] BottomSheet controls #17569

Merged
merged 8 commits into from
Oct 2, 2019

Conversation

jbinda
Copy link
Contributor

@jbinda jbinda commented Sep 25, 2019

Description

PR is connected with #1365 in gutenberg-mobile.

Please also refer to:

Related gutenberg-mobile PR
merge this PR first [RNMobile] add RangeControl mobile implementation and [RNMobile] refactor InspectorControls

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?

Manual test on Android and iOS plus CI tests

Types of changes

Adds new layer on top of BottomSheet.Cells to use controls placed in BottomSheet in the same way as Sidebar controls 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.

@gziolo gziolo 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 Sep 26, 2019
@pinarol pinarol requested a review from etoledom September 27, 2019 11:00
@jbinda jbinda force-pushed the rnmobile/bottomsheet-controls branch from 1e8640f to 5aafa7d Compare October 1, 2019 10:02
@jbinda jbinda changed the base branch from rnmobile/master to master October 1, 2019 10:04
@jbinda jbinda marked this pull request as ready for review October 1, 2019 10:05
@jbinda
Copy link
Contributor Author

jbinda commented Oct 1, 2019

@etoledom I have open PR to review and target to master branch, can you take a quick look once again ?

@jbinda jbinda changed the base branch from master to rnmobile/master October 1, 2019 10:22
@jbinda jbinda changed the base branch from rnmobile/master to master October 1, 2019 10:22
Copy link
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.

Nice job @jbinda !

Tested TextControl, ToggleControl and SelectControl on both iOS and Android and they just work beautifully 🎉

Note: Added a comment on code to be fixed before merging, but overall looks great.

Regarding the failing test, it's a known issue that a test regarding autosave might fail. #17652 (comment)

@etoledom
Copy link
Contributor

etoledom commented Oct 2, 2019

I've re-triggered CI (a few times) now they are all passing 🎉

@jbinda
Copy link
Contributor Author

jbinda commented Oct 2, 2019

thanks ! shall we merge it ? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants