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.

Sign up for GitHub

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

Jump to bottom

Mobile: BottomSheet design tweaks v2 #13855

Merged
merged 21 commits into from
Feb 18, 2019
Merged

Mobile: BottomSheet design tweaks v2 #13855

merged 21 commits into from
Feb 18, 2019

Conversation

etoledom
Copy link
Contributor

@etoledom etoledom commented Feb 13, 2019 •
edited
Loading

Description

This PR improves the design of the BottomSheet following wordpress-mobile/gutenberg-mobile#535 .
Also added fixed for:
wordpress-mobile/gutenberg-mobile#597
wordpress-mobile/gutenberg-mobile#596

  • Added more space at the top of the BottomSheet (it should sum up to 24)
  • Make last row’s border-bottom full-width.
    • Added separator styles: fullWidth, leftMargin, and none. If it's not specified, it will behave as before.
    • @marecar3 - This change might hit the BottomSheet on your PR too.
  • Truncate long text strings in the middle instead of at the end of the line.
    • This one was tricky since TextInput doesn't have this configuration. I ended up replacing the TextInput with a Text component to display the value, and change it back to TextInput when the user want's to edit it.
  • Force title case for the row titles.
    • We don't have a "native" way of doing it in RN (not supported), so I opted for doing it manually for now.

bottom-sheet-design

To test:

Swipe:

  • Test swipe to dismiss from anywhere in the BottomSheet.

@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 Feb 13, 2019
@etoledom etoledom self-assigned this Feb 13, 2019
@etoledom etoledom mentioned this pull request Feb 13, 2019
Closed
@etoledom etoledom requested a review from marecar3 February 13, 2019 12:59
This was referenced Feb 14, 2019
Merged
Merged
@etoledom
Copy link
Contributor Author

Hey @marecar3 ! I added some extra commits to implement the picker design by Thomas. This is how it looks now on Android (while iOS has no visual changes):

android

By default those changes fixed wordpress-mobile/gutenberg-mobile#597

And I also took the opportunity to fix:
wordpress-mobile/gutenberg-mobile#596

I updated the description with a new test step.

@marecar3
Copy link
Contributor

Thanks for the update @etoledom. I will take a look :)

@iamthomasbishop
Copy link

@etoledom One thing I just thought about regarding the case where we're using the Bottom Sheet component as a replacement/equivalent to iOS ActionSheets – we might want to consider removing the drag handle, as there is no real need to have the handle for this type of quick item selection interaction.

Visual example:

image

  • 1: full sheet
  • 2, 3: "Action" Sheets

Something to consider moving forward.

@etoledom
Copy link
Contributor Author

Thank you @marecar3 for the review and for the fix!
I have updated gutenberg-mobile and it should be passing now 🙏

marecar3
marecar3 approved these changes Feb 18, 2019
View reviewed changes
Copy link
Contributor

@marecar3 marecar3 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 0f84a81 into master Feb 18, 2019
@etoledom etoledom deleted the rnmobile/bottom-sheet-design-v2 branch February 18, 2019 19:18
This was referenced Feb 19, 2019
Closed
Closed
Closed
@youknowriad youknowriad added this to the 5.2 (Gutenberg) milestone Mar 4, 2019
@ellatrix ellatrix mentioned this pull request Mar 4, 2019
Merged
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@etoledom @youknowriad
Mobile: BottomSheet design tweaks v2 (#13855)
8d53d23 
* Mobile BottomSheet: Added the posibility of selecting the cell  separator style.

* Mobile BottomSheet: Increased space between top and table

* Mobile BottomSheet: Truncating long values at the middle of the string.

* Fix lint issues

* Fix syntax error to pass CI

* Fix lint issue

* Mobile Picker: Tweak Android styles.

* Mobile BottomSheet: Simplified Android cell styling.

* Mobile: Fix swipe to dismiss on BottomSheet

* Fix lint issues

* Mobile BottomSheet: Fixes sensibility of pan vs tap gestures on swipe to dismiss.

* Fix lint issues

* Fixed failed tests

* Mobile BottomSheet cell: Removed unnecessary variable init.

* Fixed set state after component is unmounted

* Revert "Fixed set state after component is unmounted"

This reverts commit 787df73.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@etoledom @youknowriad
Mobile: BottomSheet design tweaks v2 (#13855)
b8fcb2d 
* Mobile BottomSheet: Added the posibility of selecting the cell  separator style.

* Mobile BottomSheet: Increased space between top and table

* Mobile BottomSheet: Truncating long values at the middle of the string.

* Fix lint issues

* Fix syntax error to pass CI

* Fix lint issue

* Mobile Picker: Tweak Android styles.

* Mobile BottomSheet: Simplified Android cell styling.

* Mobile: Fix swipe to dismiss on BottomSheet

* Fix lint issues

* Mobile BottomSheet: Fixes sensibility of pan vs tap gestures on swipe to dismiss.

* Fix lint issues

* Fixed failed tests

* Mobile BottomSheet cell: Removed unnecessary variable init.

* Fixed set state after component is unmounted

* Revert "Fixed set state after component is unmounted"

This reverts commit 787df73.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marecar3 marecar3 marecar3 approved these changes

Assignees

@etoledom etoledom

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
5.2 (Gutenberg)
Development

Successfully merging this pull request may close these issues.
4 participants
@etoledom @marecar3 @iamthomasbishop @youknowriad