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

Mobile bottom sheet component #13612

Merged
merged 18 commits into from
Feb 1, 2019
Merged

Conversation

etoledom
Copy link
Contributor

@etoledom etoledom commented Jan 31, 2019

Description

This PR introduces a BottomSheet component to be used on mobile. It's a special use-case of the react-native-modal component implementing our design and animations.

It has props for the header's left and right buttons and title, and the onClose action.

It's firstly implemented as the Image Settings interface for the Image block.

bottom-sheet

To test:

  • Checkout the gutenberg-mobile side PR (Bottom sheet component (Image Settings) wordpress-mobile/gutenberg-mobile#529).
  • Build and run the example project.
  • Select an image block with image.
  • Press the settings button (dented wheel) in the inline toolbar.
  • Check that the bottom sheet appears.
  • Check that the bottom sheet dismiss with when
    • Tapping on the Done button.
    • Tapping on the background.
    • Sliding it down.

@etoledom etoledom 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 Jan 31, 2019
@etoledom etoledom self-assigned this Jan 31, 2019
@etoledom etoledom requested a review from Tug January 31, 2019 08:43
onClose={ onImageSettingsClose }
rightButtonConfig={ { text: __( 'Done' ), color: '#0087be', onPress: onImageSettingsClose } }
>
<TouchableOpacity style={ inspectorStyles.bottomSheetCell } onPress={ () => { } }>
Copy link
Contributor

Choose a reason for hiding this comment

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

This could probably be part of BottomSheet which would have its default style and could accept a prop containerStyle or style to customize it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean, similar to the Button above?
To have a Cell kind-of component exposed from BottomSheet?

Copy link
Contributor

@Tug Tug Jan 31, 2019

Choose a reason for hiding this comment

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

I meant more like update the BottomSheet render method to something like:

render() {
	const { isVisible, leftButtonConfig, rightButtonConfig, containerStyle, onContainerPress } = this.props;
 	return (
		<Modal
			....
			<View style={ styles.separator } />
			<TouchableOpacity style={ { ... defaultContainerStyle, ...containerStyle } } onPress={ onContainerPress || noop }>
				{ this.props.children }
			</TouchableOpacity>
			<View style={ { flexGrow: 1 } }></View>
			....
		</Modal>
	);
}

Copy link
Contributor Author

@etoledom etoledom Jan 31, 2019

Choose a reason for hiding this comment

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

Right, the problem there is that the buttons or cells are defined by the caller side. And each of those cells will have a TouchableOpacity component. Later on we will add more cells to it:

<BottomSheet
	isVisible={ this.state.showSettings }
	title={ __( 'Image Settings' ) }
	onClose={ onImageSettingsClose }
	rightButtonConfig={ { text: __( 'Done' ), color: '#0087be', onPress: onImageSettingsClose } }
>
	<TouchableOpacity style={ inspectorStyles.bottomSheetCell } onPress={ () => { } }>
		<Text style={ inspectorStyles.bottomSheetCellLabel }>{ __( 'Alt Text' ) }</Text>
		<Text style={ inspectorStyles.bottomSheetCellValue }>{ __( 'None' ) }</Text>
	</TouchableOpacity>
	<TouchableOpacity style={ inspectorStyles.bottomSheetCell } onPress={ () => { } }>
		<Text style={ inspectorStyles.bottomSheetCellLabel }>{ __( 'Size' ) }</Text>
		<Text style={ inspectorStyles.bottomSheetCellValue }>{ __( 'Full Size' ) }</Text>
	</TouchableOpacity>
</BottomSheet>

That's why I thought that maybe extracting the whole cell as a small component would be good:

<TouchableOpacity style={ inspectorStyles.bottomSheetCell } onPress={ () => { } }>
	<Text style={ inspectorStyles.bottomSheetCellLabel }>{ __( 'Size' ) }</Text>
	<Text style={ inspectorStyles.bottomSheetCellValue }>{ __( 'Full Size' ) }</Text>
</TouchableOpacity>

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, so the TouchableOpacity is for a "cell" I thought it was for the whole content. Yeah makes sense. I'm not sure the concept of a cell needs to be part or BottomSheet but it looks like a nice addition at this point 👍

@etoledom
Copy link
Contributor Author

Hey @Tug - Thanks for the review!
I implemented the requested changes on Buttons, and my idea about the Cell component so you can check it out.

Now when we need more cells we just do:

<BottomSheet.Cell label={ __( 'Alt Text' ) } value={ __( 'None' ) } onPress={ () => {} } />
<BottomSheet.Cell label={ __( 'Second' ) } value={ __( 'Something else' ) } onPress={ () => {} } />

And it will look like this:
simulator screen shot - iphone x - 2019-01-31 at 20 56 17

Let me know if you like that.

@etoledom
Copy link
Contributor Author

etoledom commented Feb 1, 2019

@Tug - Another info: We will continue to iterate in the design and implementation of the BottomSheet.

I'm already working on:

  • Adding icons to the Cell, and
  • Making it possible to show just the label with text centered.
  • An option to completely hide the header from the BottomSheet.

All these on a separate PR (based on this one)

Copy link
Contributor

@Tug Tug left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@etoledom etoledom merged commit c20a644 into master Feb 1, 2019
@etoledom
Copy link
Contributor Author

etoledom commented Feb 1, 2019

Thank you!

daniloercoli added a commit that referenced this pull request Feb 1, 2019
…rnmobile/372-use-RichText-on-Title-block

* 'master' of https://github.com/WordPress/gutenberg:
  Try alternate list item jump fix. (#12941)
  Mobile bottom sheet component (#13612)
  Remove unintentional right-margin on last odd-item. (#12199)
  Introduce left and right float alignment options to latest posts block (#8814)
  Fix Google Docs table paste (#13543)
  Increase bottom padding on gallery image caption (#13623)
  Fix the editor save keyboard shortcut not working in code editor view (#13159)
  Plugin: Deprecate gutenberg_add_admin_body_class (#13572)
  Rnmobile/upload media failed state (#13615)
  Make clickOnMoreMenuItem not dependent on aria labels (#13166)
  Add: className prop support to server side render. (#13568)
  Fix: Categories Block: hierarchical Dropdown (#13567)
  Docs: Add clarification about git workflow (#13534)
  Plugin: Remove `user_can_richedit` filtering (#13608)
  eslint-plugin: Add rule `no-unused-vars-before-return` (#12828)
  Image settings button (#13597)
  Fixed wording for the color picker saturation (#13479)

# Conflicts:
#	packages/block-library/src/image/edit.native.js
@etoledom etoledom deleted the rnmobile/bottom-sheet-component branch February 1, 2019 10:31
@youknowriad youknowriad added this to the 5.0 (Gutenberg) milestone Feb 4, 2019
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* rnmobile: Implement image settings button using InspectorControls.Slot pattern.

* rnmobile: Add missing semicolon

* rnmobile: Adding bottom-sheet component to mobile

* rnmobile: Styling bottom-sheet header

* rnmobile: Bottom-sheet clean up

* rnmobile: Fix lint issues on bottom-sheet related code.

* rnmobile: Fix small lint issues

* rnmobile: Move inline toolbar button definition out of constant.

* rnmobile: Remove extra white-spaces

* revert package-lock.json changes

* rnmobile: Fix merge issue

* rnmobile: exporting component BottomSheet.Button to be used as bottom-sheet header buttons

* rnmobile: Adding BottomSheet.Cell component as an extraction for BottomSheet users.

* Fix lint issues

* Reverting changes to package-lock.json

* Fix merge issues
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* rnmobile: Implement image settings button using InspectorControls.Slot pattern.

* rnmobile: Add missing semicolon

* rnmobile: Adding bottom-sheet component to mobile

* rnmobile: Styling bottom-sheet header

* rnmobile: Bottom-sheet clean up

* rnmobile: Fix lint issues on bottom-sheet related code.

* rnmobile: Fix small lint issues

* rnmobile: Move inline toolbar button definition out of constant.

* rnmobile: Remove extra white-spaces

* revert package-lock.json changes

* rnmobile: Fix merge issue

* rnmobile: exporting component BottomSheet.Button to be used as bottom-sheet header buttons

* rnmobile: Adding BottomSheet.Cell component as an extraction for BottomSheet users.

* Fix lint issues

* Reverting changes to package-lock.json

* Fix merge issues
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.

None yet

3 participants