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] Page Template Picker UI Improvements #19307

Merged
merged 5 commits into from
Jan 7, 2020

Conversation

geriux
Copy link
Member

@geriux geriux commented Dec 23, 2019

Gutenberg-mobile PR -> wordpress-mobile/gutenberg-mobile#1720

Description

This PR updates the current styles for the Page Template picker. It also includes some code changes to support more items in the list and accessibility.

To test

  • Open WP iOS / WP Android
  • Go to Site Pages
  • Add a new page
    • For iOS Tap on the Add button in the header (top right)
  • Expect the page template picker to be shown with the styles from the screenshots
  • Tap on either predefined layout
  • Expect the content of the page to change and the picker to disappear
iOS Android

Screenshots

iOS

Default Dark mode
iPad Default iPad Dark mode

Android

Types of changes

  • UI Styling
  • Accessibility

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@geriux geriux 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 Dec 23, 2019
const butonTextStyles = getStylesFromColorScheme( styles.buttonText, styles.buttonTextDark );
const accessibilityLabel = sprintf(
/* translators: accessibility text. Inform about list of predefined layout options. %1$s: Layout name, e.g About. */
__( 'Predefined layout picker. %1$s' ),
Copy link
Member Author

Choose a reason for hiding this comment

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

After reading this https://make.wordpress.org/design/2019/11/14/blocks-patterns-and-layouts/ I used Layout instead of Page template.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I wonder if we should start renaming things more broadly (file and component names)

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like too much text before the actual button label. I'm not sure I would specify an accessibilityLabel here. Instead, we can add an accessibilityHint="Double tap to select layout".

Ideally we should find a way to convey that this is a list of possible layouts rather than individual buttons, but I don't see any support for accessibility containers in React Native yet. Since we only have a couple buttons now, it's not a big deal, but something to keep in mind for the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! I'll use accessibilityHint instead. I hope they add support for that, it'd be super great.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated 4c1c326

@geriux geriux requested a review from koke December 23, 2019 15:28
@geriux geriux added the [Status] In Progress Tracking issues with work in progress label Dec 23, 2019
@geriux geriux removed the [Status] In Progress Tracking issues with work in progress label Dec 23, 2019
@geriux geriux marked this pull request as ready for review December 23, 2019 16:42
@koke
Copy link
Contributor

koke commented Dec 26, 2019

@iamthomasbishop are we going with Twemoji for the icons or should we use the native emoji on each platform?

If we are, we should probably add twemoji as a proper dependency and try to get the SVG from there, or at least ensure we do proper attribution.

@@ -129,10 +129,10 @@ class Layout extends Component {
parentHeight={ this.state.rootViewHeight }
style={ toolbarKeyboardAvoidingViewStyle }
>
{ showPageTemplatePicker && <__experimentalPageTemplatePicker /> }
Copy link
Contributor

Choose a reason for hiding this comment

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

In #19106 I was also moving the picker to the bottom, although I guess it makes more sense to do it as part of this PR.

Copy link
Contributor

@koke koke left a comment

Choose a reason for hiding this comment

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

This is looking pretty nice 👌 From my side, it's good to go after the accessibility changes. I can't request a review from @iamthomasbishop directly, but I'd wait for him to check that the changes look right

@koke koke mentioned this pull request Dec 27, 2019
6 tasks
@geriux
Copy link
Member Author

geriux commented Dec 31, 2019

This is looking pretty nice 👌 From my side, it's good to go after the accessibility changes. I can't request a review from @iamthomasbishop directly, but I'd wait for him to check that the changes look right

Thanks!! Yup, I'll wait to check with @iamthomasbishop about your comment above about the icons and that the changes look ok. 👍

@iamthomasbishop
Copy link

Hey @geriux – looking good! I just have a few notes:

  • I was going to say the chip/button background seemed a bit too dark, then I realized I accidentally bumped the background colors to a stronger shade of gray 🤦‍♂ I've updated the components in the design docs. (black @ 4% in light mode, white @ 8% in dark mode)

  • Ideally, we can just use the native emoji if possible // cc @koke

  • What happens to the chips/buttons if I hide the keyboard? Do they stay fixed in the middle of the screen, or move down with the keyboard/toolbar?

@geriux
Copy link
Member Author

geriux commented Jan 3, 2020

Hey @geriux – looking good! I just have a few notes:

Thanks for reviewing it @iamthomasbishop!

  • I was going to say the chip/button background seemed a bit too dark, then I realized I accidentally bumped the background colors to a stronger shade of gray 🤦‍♂ I've updated the components in the design docs. (black @ 4% in light mode, white @ 8% in dark mode)

Haha :D no problem, I've just updated the code and the screenshots above, let me know if they look ok now.

  • Ideally, we can just use the native emoji if possible // cc @koke

Changed for the native emoji

  • What happens to the chips/buttons if I hide the keyboard? Do they stay fixed in the middle of the screen, or move down with the keyboard/toolbar?

Right now they move down, let me know if this is ok or if they should stay fixed:

@iamthomasbishop
Copy link

@geriux awesome! Colors, emoji, and positioning are all looking great!

@geriux geriux merged commit 73994a8 into master Jan 7, 2020
@geriux geriux deleted the rnmobile/page-template-picker-styles branch January 7, 2020 13:58
@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
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