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

BottomSheet centered with maximum width. #600

Merged
merged 11 commits into from
Feb 19, 2019

Conversation

etoledom
Copy link
Contributor

Fixes: #572

centered-bottom-sheet
simulator screen shot - ipad pro 9 7-inch - 2019-02-14 at 16 45 25

This PR updates the gutenberg ref to test WordPress/gutenberg#13882

Note: Don't worry about the separators going out of the bottom sheet. That is resolved here: WordPress/gutenberg#13855

To test:
Please refer to that PR for testing.

cc @iamthomasbishop

@etoledom etoledom added the [Type] Enhancement Improves a current area of the editor label Feb 14, 2019
@etoledom etoledom added this to the Beta milestone Feb 14, 2019
@etoledom etoledom self-assigned this Feb 14, 2019
@etoledom etoledom changed the title Update gutenberg ref BottomSheet centered with maximum width. Feb 14, 2019
@iamthomasbishop
Copy link
Contributor

This is looking great, @etoledom! Assuming the border-width issue and scrollable issues are being addressed separately, this one looks good to go.

@etoledom etoledom requested a review from pinarol February 19, 2019 09:01
Copy link
Contributor

@pinarol pinarol 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 handling the inserter ui refactoring also! This looks great. Tested both with inserter ui and image settings ui, works pretty good on both safe area and normal screens on portrait/landscape modes. Left just a few tiny comments about deleting the unused items.

import styles from './block-picker.scss';
import { name as unsupportedBlockName } from '../block-types/unsupported-block';
// Gutenberg imports
import { getBlockTypes } from '@wordpress/blocks';

type PropsType = {
style?: StyleSheet,
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we don't need safeAreaBottomInset anymore

and we can also get rid of this at the bottom of the page:

BlockPicker.defaultProps = {
safeAreaBottomInset: 0,
};

@@ -30,57 +36,46 @@ export default class BlockPicker extends Component<PropsType> {

render() {
const numberOfColumns = this.calculateNumberOfColumns();
const paddingBottom = this.paddingBottom();
const bottomPadding = this.props.addExtraBottomPadding && styles.contentBottomPadding;
Copy link
Contributor

Choose a reason for hiding this comment

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

we can add addExtraBottomPadding to PropsType

@etoledom
Copy link
Contributor Author

Since this PR was so small, I took the opportunity to replace the BlockPicker Modal with the BottomSheet component, as a follow up of #608

cc @iamthomasbishop

picker

@etoledom etoledom merged commit 2a715ce into develop Feb 19, 2019
@etoledom etoledom deleted the issue/bottom-sheet-width-centered branch February 19, 2019 10:20
@etoledom
Copy link
Contributor Author

Thank you!

@iamthomasbishop
Copy link
Contributor

Looks great! One minor thing, but we can address it separately – the left/right sheet margins on the smaller portrait Android example are a bit narrow. Based on the screenshots I'm guessing we're using a static width for each block type which is fine for now, but we might want to adjust it to use percentages at some point to accommodate narrow screens.

@etoledom
Copy link
Contributor Author

etoledom commented Feb 19, 2019

I'm guessing we're using a static width for each block

Yes, width is static. If the screen is thin enough, the number of columns will be reduced to 2.
Of course, we can iterate and improve this :)

@iamthomasbishop
Copy link
Contributor

Awesome, thanks for the confirmation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement BottomSheet max width and centered.
3 participants