Skip to content

[RNMobile] BottomSheet controls#17569

Merged
etoledom merged 8 commits intoWordPress:masterfrom
callstack:rnmobile/bottomsheet-controls
Oct 2, 2019
Merged

[RNMobile] BottomSheet controls#17569
etoledom merged 8 commits intoWordPress:masterfrom
callstack:rnmobile/bottomsheet-controls

Conversation

@jbinda
Copy link
Copy Markdown
Contributor

@jbinda jbinda commented Sep 25, 2019 •
edited
Loading

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.

@jbinda jbinda mentioned this pull request Sep 25, 2019
Merged
1 task
@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
Copy Markdown
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
etoledom
etoledom approved these changes Oct 1, 2019
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.

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
Copy Markdown
Contributor

etoledom commented Oct 2, 2019 •
edited
Loading

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

@jbinda
Copy link
Copy Markdown
Contributor Author

jbinda commented Oct 2, 2019

thanks ! shall we merge it ? :)

@etoledom etoledom merged commit d75119d into WordPress:master Oct 2, 2019
@youknowriad youknowriad added this to the Gutenberg 6.7 milestone Oct 14, 2019
@cameronvoell cameronvoell mentioned this pull request Jan 17, 2020
Open
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

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 6.7

Development

Successfully merging this pull request may close these issues.

4 participants

@jbinda @etoledom @youknowriad @gziolo