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

Video block ui/ux enhancements #979

Merged
merged 26 commits into from
May 21, 2019
Merged

Conversation

marecar3
Copy link
Contributor

@marecar3 marecar3 commented May 13, 2019

Fixes : #966

Android Upload Video Example :

video_android

Android Upload Image Example :

image_android

More details regarding testing and preview of what is done: WordPress/gutenberg#15551

@marecar3 marecar3 added the [Type] Bug Something isn't working label May 13, 2019
@marecar3 marecar3 added this to the v1.5 milestone May 13, 2019
@marecar3 marecar3 self-assigned this May 13, 2019
@pinarol pinarol modified the milestones: v1.5, v1.6 May 15, 2019
@pinarol
Copy link
Contributor

pinarol commented May 16, 2019

hey Marko 👋 I think we might have a side effect about not showing the image placeholder. Normally I should be seeing an image placeholder during the time image loads from media library:

image-block-placeholder

@marecar3
Copy link
Contributor Author

Hey @pinarol , thanks for the feedback, I will look at it.

@iamthomasbishop
Copy link
Contributor

@marecar3 Can you please add some screenshots or videos when it's ready for design review? It's a bit hard to see what to review without visuals. Thank you!

@marecar3
Copy link
Contributor Author

marecar3 commented May 16, 2019

Hey @iamthomasbishop you can find it on this PR: WordPress/gutenberg#15551

But I will upload here newest videos for both image and video block. Will tag you when that happens.

@marecar3
Copy link
Contributor Author

hey Marko 👋 I think we might have a side effect about not showing the image placeholder. Normally I should be seeing an image placeholder during the time image loads from media library:

image-block-placeholder

Fixed! :)

@marecar3
Copy link
Contributor Author

Hey @iamthomasbishop I have updated description with the latest video presenting image/video block feature.

@pinarol
Copy link
Contributor

pinarol commented May 17, 2019

hey Marko 👋 Unfortunately that new icon/index.native.js has a huge regression effect on all types of icons. Demo apps are already crashing

Screen Shot 2019-05-17 at 11 19 19

It looks to me that we shouldn't diverse the mobile side implementation of Icon for this PR, it needs wider testing. We have our own separate js files already for icons like:
image/icon-retry.js
video/icon-retry.js

would that be possible to change those files only and pass the fill color or size if needed? I'll try to look into this a bit.

@pinarol
Copy link
Contributor

pinarol commented May 17, 2019

@marecar3 👋 I looked into that last crash a bit and I think I found a solution that won't effect other icons, let's discuss and I can push the commit if that's OK for you.

@pinarol pinarol self-assigned this May 17, 2019
@pinarol pinarol added [Status] Needs Design Review Needs design review or sign-off before shipping Media [OS] iOS [OS] Android and removed [Type] Bug Something isn't working labels May 17, 2019
@iamthomasbishop
Copy link
Contributor

Looking at the updated videos above, some more feedback:

Video upload progress

Looks a lot cleaner, well done! I noticed there is a split second before the placeholder/uploading state shows, where the inner block collapses. Can we make sure the placeholder shows immediately, so there isn't a jump?

Placeholder/uploading icons

image

(same concept applies to Image block)

For both video and image block uploading (any progress states), can we make sure:

  • Color of icon is $gray-lighten-20
  • Icon is 40x40

Transitions

I understand this might be tricky, but it would be great if we could add some transitions when media items are loaded. For example, we could:

  • Smoothly transition the height of the inner blocks when the uploaded item is being displayed
  • Fade in the video – right now it's just an immediate display upon entrance

Side Notes

@pinarol pinarol mentioned this pull request May 17, 2019
@pinarol
Copy link
Contributor

pinarol commented May 17, 2019

hey @iamthomasbishop thank you for the feedbacks, I think it is ready for another look.

noticed there is a split second before the placeholder/uploading state shows, where the inner block collapses. Can we make sure the placeholder shows immediately, so there isn't a jump?

I investigated the issue but couldn't figure the root cause, it is pretty weird that it only happens on Android. iOS part seems OK:

video-block-upload

So I added an item here #687 (comment) It'd be better if an Android dev investigates it. I am assuming this is not a blocker for shipping the video block, please let me know if you have concerns about it.

For both video and image block uploading (any progress states), can we make sure:
Color of icon is $gray-lighten-20
Icon is 40x40

You are right this wasn't correct, I updated this:

Screen Shot 2019-05-17 at 23 41 28

Caption: there is some extra padding top/bottom on the caption, but this might be related to an issue I reported earlier

I updated caption padding due to designs:

Screen Shot 2019-05-17 at 23 20 51

Screen Shot 2019-05-17 at 23 20 43

Transitions

Yes transitions are tricky, I think we might not be able to make it until v2. Plus, considering the very little usage of video blocks it might be an over effort currently. But feel free to open a new issue for it and we can estimate and plan how to do it for future releases, does that sound good?

@iamthomasbishop
Copy link
Contributor

Thanks @pinarol – these updates look great! From what I can tell from your iOS videos, everything looks to be working properly. Let's fix that issue on Android as soon as we can, but not a blocker for shipping the block.

@pinarol
Copy link
Contributor

pinarol commented May 20, 2019

thank you @iamthomasbishop I've opened an issue to track that: #1005

Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

Tested with WPiOS, WPAndroid and example apps, also got the design approval from @iamthomasbishop

@pinarol pinarol merged commit 95add06 into develop May 21, 2019
@pinarol pinarol deleted the fix/video-block-ui-ux-enhancements branch May 21, 2019 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Media [OS] Android [OS] iOS [Status] Needs Design Review Needs design review or sign-off before shipping
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Video block - UI/UX Enhancements
3 participants