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] Allow shrink toolbar #22132

Merged
merged 19 commits into from
May 22, 2020
Merged

Conversation

lukewalczak
Copy link
Member

@lukewalczak lukewalczak commented May 6, 2020

Description

Fixes: wordpress-mobile/gutenberg-mobile#2180

Ref to:
gutenberg-mobile: wordpress-mobile/gutenberg-mobile#2220

That PR introduces the possibility to stack options in into one toolbar button which will open an ActionSheet (ios) / BottomSheet (Android).

How has this been tested?

  1. Open mobile app
  2. Add Buttons block
  3. Focus Button block inside Buttons block
  4. Expect to see stacked options in the mobile toolbar
  5. Press button in the mobile toolbar
  6. Expect the disabled button Move Left for the first button
  7. Expect the disabled button Move Right for the last button
  8. Try all the options:
    • expect Button is moving left/right when these options are available
    • expect to be able to open settings
    • expect to be able to remove block

Screenshots

ios android
Screenshot 2020-05-05 at 16 57 15 Screenshot 2020-05-05 at 14 09 44

Types of changes

  • The new type of mobile toolbar named ShrinkBlockMobileToolbar along with new prop allowShrinkToolbar.

  • Add option to disable buttons within Android Picker.

  • Two new options to iOS Picker called: disabledButtonIndices and destructiveButtonIndex

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.

@lukewalczak lukewalczak changed the title Rnmobile/allow shrink toolbar [RNMobile] Allow shrink toolbar May 6, 2020
@lukewalczak lukewalczak self-assigned this May 6, 2020
@lukewalczak lukewalczak added [Block] Buttons Affects the Buttons Block Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels May 6, 2020
@lukewalczak
Copy link
Member Author

Hello @iamthomasbishop, I'm wondering how disabled buttons should look on Android, could you please specify that? Also on the initial mock for Android, there was a separator between Block Settings and Remove Block, is it valid?

@iamthomasbishop
Copy link

I'm wondering how disabled buttons should look on Android

@lukewalczak Ideally, we should hide the button altogether if it's not actionable (at least on Android — that doesn't seem possible on iOS from what I can tell?). If we can't hide it altogether, then it can use a very dim color (very light in light mode and very dark in dark mode).

We should probably re-use existing styles if possible, but I'm not 100% sure how disabled buttons/icons are styled in code. Based on a quick analysis by pulling screenshots into Figma, it looks like disabled inline toolbar buttons are something like ~30% opacity of our default icon color (or maybe that's a solid color? not sure).

My suggestion would be to match the icon color with the disabled state of the up/down movers on the inline toolbar. If that is indeed 30% opacity, then maybe we should simply dim the entire cell row to 30% alpha and roll with that. That would look something like this (ignore the icons in this mock):

Also on the initial mock for Android, there was a separator between Block Settings and Remove Block, is it valid?

That would be ideal because I think some separation between the primary actions and a destructive action would be nice. If we're going to add a separator, I would add 8px of space above and below the separator. This is how it would look all together:

Let me know if you have any questions or if that doesn't make sense. 😄

@iamthomasbishop
Copy link

@lukewalczak Also, one question I have regarding the scope of this feature: Are we making the ••• toolbar button and menu universal, to all inline toolbars? Or only when it is collapsed because the block is narrow? Maybe a visual will help (forgive the sloppy sketch 😬):

image

With that example in mind, is this ticket aiming to:

A) Collapse into a ••• menu only when the block is very narrow? Or...
B) Always show the ••• menu, and collapse the items into the menu when the block is very narrow?

I think the end goal is to have something like B, but I’m not 100% sure if that menu should house all block actions, or only the “overflow” ones when the toolbar is collapsed. I would like to at least always have the Remove block action in there because this would also help with an issue we’ve been seeing reports of, where users are accidentally deleting blocks too easily, as noted in this issue.

@lukewalczak lukewalczak force-pushed the rnmobile/allow-shrink-toolbar branch from 0e2d868 to c6bfec9 Compare May 7, 2020 18:35
@iamthomasbishop
Copy link

@lukewalczak I had a chance to review earlier today, this is feeling great already! I jotted down some notes — most of it very minor — some of which is relevant to this ticket, but also another that I will create a ticket for tomorrow.

  • As we discussed, the new ellipsis (•••) icon from wordpress/icons feels too light-weight (each circle is ~2px and the result is a little weak/thin). I've added a slightly heavier version (in horizontal and vertical variations) to the design docs, which feels more appropriate without feeling out of place relative to the other icons. Here's what the icon looks like in context.
  • Can we use the horizontal variation of the ••• icon? It feels a little better for horizontal rhythm and spacing.
  • This is counter to my sketch above, but when the block is wide enough to show the cog/settings icon, can we give first preference to showing the cog/settings icon before the arrows? So as that space expands, the settings icon would show first, then only when there is enough space to show both mover arrows would we show those.
  • Can we change the label of the Remove Block action (in the ••• menu) to Remove [Block type]? For example if I go to remove an Image block, the label would become Remove Image.
  • Not a blocker, but do we have the ability to add icons to the iOS actionSheet? (example)
  • I noticed when I have a single Buttons block that the movers are still part of the ••• menu. Shouldn't they be disabled? (example)

Not relevant to this ticket:

  • Specific to the Buttons block: I originally proposed adding a selection border to the selected button — after some time with the button I think we can remove it because the outer selection outline is sufficient (example) . I can create a ticket for this one tomorrow.

Great work! This is going to be really great to have in the editor — it feels very intuitive and should also solve this issue. // cc @mchowning

@lukewalczak
Copy link
Member Author

Hello Thomas! Thanks for the feedback:

As we discussed, the new ellipsis (•••) icon from wordpress/icons feels too light-weight (each circle is ~2px and the result is a little weak/thin). I've added a slightly heavier version (in horizontal and vertical variations) to the design docs, which feels more appropriate without feeling out of place relative to the other icons. Here's what the icon looks like in context.

Done! I've added moreHorizontalMobile icon.

Can we use the horizontal variation of the ••• icon? It feels a little better for horizontal rhythm and spacing.

Sure!

This is counter to my sketch above, but when the block is wide enough to show the cog/settings icon, can we give first preference to showing the cog/settings icon before the arrows? So as that space expands, the settings icon would show first, then only when there is enough space to show both mover arrows would we show those.

I've changed the order.

Can we change the label of the Remove Block action (in the ••• menu) to Remove [Block type]? For example if I go to remove an Image block, the label would become Remove Image.

Done!

Not a blocker, but do we have the ability to add icons to the iOS actionSheet? (example)

Nope, there is no option for that. This is native ios ActionSheet and it's not supporting that.

I noticed when I have a single Buttons block that the movers are still part of the ••• menu. Shouldn't they be disabled? (example)

Generally, it's fixed, but I released a testing build from the branch without that change.

@lukewalczak lukewalczak force-pushed the rnmobile/allow-shrink-toolbar branch from 437032b to 6479435 Compare May 8, 2020 11:44
@lukewalczak lukewalczak force-pushed the rnmobile/allow-shrink-toolbar branch from 6479435 to 3edf716 Compare May 8, 2020 12:53
@iamthomasbishop
Copy link

Thanks for the update, @lukewalczak ! Your notes make sense and most everything is working perfectly. Here are some notes based on the new build.

  • Empty post: Can we either remove or disable the ••• button on the Inline Toolbar when the user has an empty post and the first empty paragraph is selected. Note: If we remove, we can probably hide the Inline Toolbar altogether (only on the first block, when the post is empty).
  • On the actionSheet, can we change Block Settings to [Block type] Settings? For example: Cover Settings or Gallery Settings.

Relevant, but not a blocker:

  • Is it possible to make the ActionSheet a Popover on iPad so that the options display directly over the ••• toggle? (example)

Not specific to this ticket:

  • I'm not sure if this is related to this work, but I noticed the "Number of columns" table cell on the Columns block settings looks disabled. (screenshot)
  • Icons look too dim when they are "above" a cover background. (example). I think this will need some separate time for design consideration.

@github-actions
Copy link

github-actions bot commented May 11, 2020

Size Change: 0 B

Total Size: 1.12 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.93 kB 0 B
build/block-directory/style-rtl.css 790 B 0 B
build/block-directory/style.css 791 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 7.17 kB 0 B
build/block-library/editor.css 7.17 kB 0 B
build/block-library/index.js 119 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 190 kB 0 B
build/components/style-rtl.css 17.1 kB 0 B
build/components/style.css 17.1 kB 0 B
build/compose/index.js 9.24 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.42 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/index.js 6.63 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/index.js 302 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 13.2 kB 0 B
build/edit-site/style-rtl.css 5.45 kB 0 B
build/edit-site/style.css 5.45 kB 0 B
build/edit-widgets/index.js 7.87 kB 0 B
build/edit-widgets/style-rtl.css 4.58 kB 0 B
build/edit-widgets/style.css 4.57 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.6 kB 0 B
build/editor/style-rtl.css 5.06 kB 0 B
build/editor/style.css 5.06 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.64 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@lukewalczak lukewalczak force-pushed the rnmobile/allow-shrink-toolbar branch from 9c32272 to 5e5a39b Compare May 11, 2020 12:00
@lukewalczak lukewalczak force-pushed the rnmobile/allow-shrink-toolbar branch from 5e5a39b to 2b84538 Compare May 11, 2020 12:01
@lukewalczak
Copy link
Member Author

@iamthomasbishop Currently we have block name near to the action such as Remove [block name], [block name] Settings, but for moving block there is a label Move Block Left/Right. I think we should pass there block name as well. Wdyt?

@iamthomasbishop
Copy link

I think we should pass there block name as well. Wdyt?

@lukewalczak I think we probably could, yea. I'm starting to wonder how blocks with long names will look w/ these labels, esp with long translations. But as long as we're handling them well, it should be okay. Would you mind testing out what a very long block name would do in this scenario — maybe by forcing a long name?

@lukewalczak
Copy link
Member Author

Since it's a native component, it's handling that case for us and we don't have to worry about it - message font size will be decreasing until it fits in button.

For the illustration, I forced a very very long name and there are the observations:

regular device font regular device font the largest device font
Screenshot 2020-05-14 at 16 14 36 Screenshot 2020-05-14 at 16 10 05 Screenshot 2020-05-14 at 16 09 47

@iamthomasbishop
Copy link

Awesome! Then we're good 😄

@lukewalczak lukewalczak marked this pull request as ready for review May 15, 2020 07:58
@lukewalczak lukewalczak force-pushed the rnmobile/allow-shrink-toolbar branch from 6893862 to 1f34cee Compare May 19, 2020 08:08
Copy link
Contributor

@dratwas dratwas left a comment

Choose a reason for hiding this comment

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

Good job @lukewalczak !

Copy link
Member

@geriux geriux left a comment

Choose a reason for hiding this comment

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

Hey @lukewalczak 👋 So I left a couple of comments but overall is looking good!

One question, is this expected?:

I no longer see the trash icon but the "three dots" instead.

@lukewalczak
Copy link
Member Author

Hey @geriux, yes it's intended. We are aiming for a new solution to avoid unexpected block deletion.

More details in Thomas's comment

@geriux
Copy link
Member

geriux commented May 20, 2020

Hey @geriux, yes it's intended. We are aiming for a new solution to avoid unexpected block deletion.

More details in Thomas's comment

Cool! I wanted to double check =) there was quite a lot of messages to keep up. Thanks!

Copy link
Member

@geriux geriux left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work Luke! Tested it again after the changes. 👏

@lukewalczak lukewalczak force-pushed the rnmobile/allow-shrink-toolbar branch from e87622c to 2c420d8 Compare May 22, 2020 11:17
@lukewalczak lukewalczak merged commit 9ada621 into master May 22, 2020
@lukewalczak lukewalczak deleted the rnmobile/allow-shrink-toolbar branch May 22, 2020 13:46
@github-actions github-actions bot added this to the Gutenberg 8.2 milestone May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Buttons Affects the Buttons Block 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.

Allow shrinking the block toolbar for small blocks
5 participants