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

Changing Unsupported title and adding help info block (Issue #479) #17943

Closed
wants to merge 6 commits into from
Closed

Changing Unsupported title and adding help info block (Issue #479) #17943

wants to merge 6 commits into from

Conversation

illusaen
Copy link
Contributor

@illusaen illusaen commented Oct 14, 2019

Description

  • Changed text on Unsupported block to show original block name.
  • Added a help icon that shows additional information when the block is clicked.

How has this been tested?

  • Tested on simulator and on Android device.
  • Wrote unit tests for component change.
  • Component change affects all types of blocks that were defaulting to the 'Missing' block (aka UnsupportedBlockEdit)

Screenshots

unsupported

Types of changes

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.

…ng original name in place of 'Unsupported'.
…ause the small icon was too hard to press. Also rearranging the BottomSheet to work the height paddings out correctly.
@gziolo gziolo added First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Feature] Blocks Overall functionality of blocks labels Oct 15, 2019
@SergioEstevao
Copy link
Contributor

@iamthomasbishop do we want to have a close button in the BottomSheet?

@SergioEstevao
Copy link
Contributor

@iamthomasbishop and @illusaen shouldn't the bottom sheet only show up when pressing the ? icon?

@iamthomasbishop
Copy link

@SergioEstevao That was the original idea, but considering there aren’t any other actions on the unsupported blocks, I don’t think there’s any harm in making the whole block trigger this sheet. If we have cases where there is another action attached to the block, we should make it so just a 44x44 area containing the icon is tappable.

@illusaen One tiny thing I just realized, I might have used the wrong icon for the sheet mockup. We should use the same “question mark” icon in the sheet that we use in the block on the canvas.

@SergioEstevao
Copy link
Contributor

but considering there aren’t any other actions on the unsupported blocks

@iamthomasbishop before this PR you could at least move the blocks up and down on the post. It would be great to keep that ability and only have the info show when we tap the ? icon.

@iamthomasbishop
Copy link

It would be great to keep that ability and only have the info show when we tap the ? icon.

@SergioEstevao I should have been more specific about what I was imagining. I meant that tapping anywhere on the inner body of the block (gray background) could perform this action, but only after the block is selected.

In other words, this is what I was imagining. With the block un-selected:

  • First tap would select the block (and show the inline toolbar controls for up/down, etc)
  • Once selected, tapping on the body would perform the action

With that said, I think you're right that we might only want the (?) button/icon to be the trigger.

@iamthomasbishop
Copy link

Side note: media placeholder blocks also suffer from a similar issue regarding the move controls. For example, if you have an empty image or video block on the canvas, the first tap always shows the media source sheet. In other words, you cannot select and then move the block.

@illusaen
Copy link
Contributor Author

Side note: media placeholder blocks also suffer from a similar issue regarding the move controls.

That's actually where I got the idea. Would you like me to change Media Placeholder as well?

@iamthomasbishop
Copy link

That's actually where I got the idea. Would you like me to change Media Placeholder as well?

There are potentially a half dozen places where this would apply so we might want to create a separate issue for it and discuss what needs to be done where before adding to this task.

Oddly enough, the image block is behaving slightly differently today than it was when I tested yesterday – maybe a bug, but I still feel like placeholders should always require a tap to select then another to take the action to ensure the controls are always toggled.

@SergioEstevao
Copy link
Contributor

That's actually where I got the idea. Would you like me to change Media Placeholder as well?

@illusaen let's keep this PR limited to the unsupported blocks, unless the change needed for this needs to be done in a central place.

@SergioEstevao
Copy link
Contributor

SergioEstevao commented Oct 16, 2019

@illusaen on iOS 13 the bottom sheet that you are presenting is not taking in account the dark/light mode of the OS. So in dark mode it's looking like this:

Simulator Screen Shot - iPhone 11 - 2019-10-16 at 22 51 09

Have a look at the method withPreferredColorScheme on the cell.native.js file in the BottomSheet component.

@iamthomasbishop
Copy link

Good catch, @SergioEstevao. I have some other design feedback, but I think some of it is already being worked on by @illusaen – so please ping me when it's ready for a design review or if you have design-related questions, @illusaen!

@hypest
Copy link
Contributor

hypest commented Oct 25, 2019

Sorry for not adding this earlier but, following some discussions in Slack, let's also append "(unsupported)" directly on the block view to make it clear that it's not a malfunction. This is complementary to the "?" info button and popup.

In terms of styling, we can make it similar to the "Add Media" link on media blocks but with gray instead of link color. cc @iamthomasbishop.

@maxme
Copy link
Contributor

maxme commented Oct 25, 2019

Sorry for not adding this earlier but, following some discussions in Slack, let's also append "(unsupported)" directly on the block view to make it clear that it's not a malfunction. This is complementary to the "?" info button and popup.

I opened a PR for this here: #18107

@illusaen you can eventually cherry-pick the changes or wait for the patch to be merged.

@hypest
Copy link
Contributor

hypest commented Nov 4, 2019

Just for clarity, as far as I understand the feedback/issues currently pending are:

  1. Trigger the info bottom modal only when tapping on the ? icon
  2. Dark mode compatibility of the info bottom modal
  3. "Edit in Safari" needs adaptation for Android, at least its copy

By the way @SergioEstevao , I don't see conversation about the "Edit in Safari" button in this PR. Does this feature needs to be part of the same PR? I guess we'll need bridge changes to implement that so, a separate PR would serve better.

@SergioEstevao
Copy link
Contributor

SergioEstevao commented Nov 4, 2019 via email

…or the help block. Few color changes. Adding some text to the help block.
@illusaen
Copy link
Contributor Author

illusaen commented Nov 4, 2019

I just pushed the changes I hadn't committed before: should just be dark mode + gridicon now. Unfortunately I'm still recovering (read: sleeping!) for the next few days but I'll hopefully be back soon.

@hypest
Copy link
Contributor

hypest commented Nov 4, 2019

Thanks for pushing what's already done @illusaen !

Unfortunately I'm still recovering (read: sleeping!) for the next few days but I'll hopefully be back soon.

Take care and see you back soon 😃. In the meantime, I'll copy your commits into a new branch and open a PR so we get this merged.

  1. Trigger the info bottom modal only when tapping on the ? icon
  2. Dark mode compatibility of the info bottom modal
  3. "Edit in Safari" needs adaptation for Android, at least its copy

all points were addressed by @illusaen's 47b491b so, what I'll mainly do in a new branch/PR is to fix the merge conflict with master and enable #18107 for all unsupported blocks.

@hypest
Copy link
Contributor

hypest commented Nov 4, 2019

Here's the new PR I created to follow up #18268

@gziolo
Copy link
Member

gziolo commented Nov 28, 2020

Here's the new PR I created to follow up #18268

Let’s close this PR as it seems to be implemented and original branch no longer exists.

@gziolo gziolo closed this Nov 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository 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

6 participants