-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[Mobile] Cover block uploads failure UI #21198
[Mobile] Cover block uploads failure UI #21198
Conversation
# Conflicts: # packages/block-library/src/cover/edit.native.js
…k-uploads-dev-only
…obile/feature/cover-block-uploads-progress
…obile/feature/cover-block-uploads-failure-ui
Size Change: 0 B Total Size: 842 kB ℹ️ View Unchanged
|
Hey, @mkevins 👋 Started to review/test but I noticed that on iOS the upload progress bar is not showing (on Android it is) maybe there's a z-index issue? |
Hi @geriux 👋 thanks for taking a look. I did run into that on Android too, but solved it with this: https://github.com/WordPress/gutenberg/pull/21198/files#diff-dbbdd2c3421fbb78e7d6cc0b65c4e8b1R57. So, I wonder now whether something can be different on iOS that would make that view invisible 🤔 . Thanks for the feedback, I'll investigate it 👍 |
Hi @geriux, I added a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Tested both on iOS and Android, also double-checked the other blocks using the MediaUploadProgress
were working as expected too.
Approving code and functionality but there's still pending design/UX approval.
Nice work @mkevins!
@mkevins I've not been able to test this on a device (I can't see the additional media sources in the Android build here), but looking at the screenshots above, I have some initial feedback.
If there is only 1 failed item on the post, can we remove the
This looks pretty good. Can you provide the following info:
|
Hi @iamthomasbishop 👋 😄 thanks for the quick feedback!
This should be possible, but I believe will be needed on the native side. Since this will also be affecting other media blocks, maybe it's best that I open an issue for that one and address in a separate PR. Wdyt?
The icon itself is 20px in svg, but expands to fill its container, which is 24px. The white "border" is actually coming from the svg image, and is 2px: However, that actually becomes slightly larger than 2px since the total 20px svg expands to 24px (so roughly 2.4px).
The margins (actually padding in the outer container) are 8px from the top and right.
I just used the eye-drop tool to pick the color from your mockup ( Edit: There is now an apk for WordPress-Android with Cover upload options enabled in production, which includes the new colors. |
Sounds good to me 👍
Ok, that sounds about right regarding sizing — the frame of the icon should be 24 and it's slightly inset on the grid 2pts on each side from the looks of it). Rather than giving a solid white outline, I'm wondering if we can use a semi-transparent white (about 18% alpha) with only 1px width. That would look like this: For inspection, I've added this to the design docs here.
Perfect 👍
Upon further inspection, can we use red-40 on both light/dark modes? It sits nicely on both. One more thing that I'm curious about. We are putting this indicator on the top-right of the cell, which feels fine, but I'm curious (assuming we end up using this same pattern for other errors on other blocks) if there could be a scenario where it conflicts with other actions or UI, such as the image-editing button that sits on the top-right of an image block? If that's the case, I think we can move to the bottom-left or bottom-right of the cell — top-left being reserved for media type badge (like GIFs). |
Hi @iamthomasbishop 👋
So far, the icons we've used only have 1 color (and a background color of the container). So in previous iterations shown above, the icon was getting it's white color from the background of the container, and the To achieve your designs I've added some paths to the svg to allow independent coloring: Ideally, icons will be centralized, but it's not immediately clear what the ideal pattern is for this case (multiple colors), since it seems most (maybe all?) of the "centralized" icons (
👍 Sounds good!
I'm not sure how likely that scenario is that we'll use the same pattern for other media blocks (all the others so far have their failure state in the center, since there's nothing obstructing the view there). I tried it with the icon in the lower right, and it felt strange to me that it was so close to those other icons (block settings and remove block). Here's what it looks like: Here's what it looks like in the lower left corner (better, IMO, but maybe not as nice as the upper right): Maybe we can iterate on another PR? Wdyt? |
I think to simplify this, we can probably just remove the bordering — I hate to break up and customize an icon's construction just for something like this. Simply having the white container background that fills the empty space in the middle should be enough.
Totally agreed, I noticed this while playing around w/ the cell component in Figma. I think top-right is fine assuming we don't run into conflicts w/ other UI. In that case, we can always iterate by moving to the bottom-left, as you said. 👍 |
This reverts commit 74b978e07e7a98fcf09110935c6e779eb808f2aa.
Thanks @geriux and @iamthomasbishop for your feedback! |
Description
This PR adds UI for failed uploads in the Cover block, and also allows cancellation of ongoing uploads.
Related PRs
gutenberg-mobile
: Cover block uploads failure UI wordpress-mobile/gutenberg-mobile#2150Testing steps
Upload failure UI
Upload failure UI
Expected:
A failure indicator icon should appear in the upper-right-hand corner of the block:
Subsequently, tapping the background image should provide an option to retry the upload or remove the media:
Upload cancellation
Upload cancellation
Expected:
A dialog should be presented allowing you to cancel the upload:
Types of changes
The
renderContent
propThe states in
MediaUploadProgress
component can utilized by passing a render prop, or via functional props bound to the component's internal state machine. Some of the block's UI (e.g. handling touch) is dependent on the component's internal state. We can wrap all of the dependent code within therenderContent
prop to access this state. Alternatively, we can give the block component state, and updated it via the state change functional props (onFinishMediaUploadWithSuccess
, etc.). I chose to use the latter implementation because it seemed more versatile. TheMediaUploadProgress
renderContent
prop was made optional.Failure icon
I did not find the triangle icon used in the design mockup, so instead used the dashicon "warning" icon, which renders as a circle. The exclamation mark in that svg is actually not part of the shape, so to make it white requires setting the background to white in the icon container. I gave the container a border-radius (so it renders as a circle, instead of a square) and this results in a white circle around the icon. cc: @iamthomasbishop 👋 can you let me know if this looks ok?
Upload failed icon
Design mockup
Another notable deviation from the mockup is that the upload retry / remove dialog is not in the bottomsheet, but rather in a parent-app dialog (e.g. AlertDialog in Android). In theory, we could implement a bottomsheet menu for these options, but I believe it might require some changes on the native side. Another thing to consider is that the other media blocks currently all use this same dialog. However, if we try to use the same dialog (the current state of this PR), we are missing the retry message (
'Failed to insert media.\nPlease tap for options.'
).If not displaying the retry message is a blocker, one simple solution would be to add the retry message in a bottomsheet dialog that subsequently opens the already existing parent-app dialog. But this introduces "yet another tap" to the UX, so may not be desirable.
Checklist: