[Video] [User Story] Display file information for each of the videos stored in the video app

RESOLVED FIXED

Status

Firefox OS
Gaia::Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Sri Kasetti, Assigned: pdahiya)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
User Story:

As a user, I want to see the file type, size, resolution, date of creation and duration of the video for each of the videos stored in the video application
(Assignee)

Updated

5 years ago
Assignee: nobody → pdahiya
(Assignee)

Comment 1

5 years ago
Hi Patryk

Please attach the visual design specs for this user story. I will be needing an info icon image for this feature.

Sri, can you pl. provide details such as which strings needs to be localized while displaying video information or let me know who should i reach out for the same.

I have the specs from UX in the email. Let me know if i should attach that to the bug. 

Thanks
Flags: needinfo?(skasetti)
Flags: needinfo?(padamczyk)
(Assignee)

Comment 2

5 years ago
Including Rob for UI question on gallery view. 

when we display file size and video type we are showing it below duration in a single line. In few cases when the file size text is big e.g 2224 KB, the video type moves in the next line (See attached). Is this ok or we need to always show it in one line?
Please clarify
(Assignee)

Updated

5 years ago
Flags: needinfo?(rmacdonald)
(Assignee)

Comment 3

5 years ago
Created attachment 791377 [details]
Video app - gallery view image displaying video type in next line
(Assignee)

Comment 4

5 years ago
(In reply to Punam Dahiya from comment #2)
> Including Rob for UI question on gallery view. 
> 
> when we display file size and video type we are showing it below duration in
> a single line. In few cases when the file size text is big e.g 2224 KB, the
> video type moves in the next line (See attached). Is this ok or we need to
> always show it in one line?
> Please clarify

Got clarification from Rob on IRC and it's ok for video type text to wrap to new line in gallery view.
Flags: needinfo?(rmacdonald)
Created attachment 793597 [details]
Specs + Icon

Hi, can you please have a look at the specs, judging from the 1.1 builds, the gallery view and the player seem to be off.
Flags: needinfo?(padamczyk)
(Assignee)

Comment 6

5 years ago
Thanks Patryk, the specs look good. Grouping of videos by date in gallery view is outside the scope and will be covered in a separate bug.
(Assignee)

Comment 7

5 years ago
Created attachment 794218 [details] [review]
Pull request to display file info for videos stored in video app
Attachment #794218 - Flags: review?(dflanagan)
(Assignee)

Updated

5 years ago
Blocks: 908380
(Assignee)

Comment 8

5 years ago
(In reply to Punam Dahiya from comment #1)
> Hi Patryk
> 
> Please attach the visual design specs for this user story. I will be needing
> an info icon image for this feature.
> 
> Sri, can you pl. provide details such as which strings needs to be localized
> while displaying video information or let me know who should i reach out for
> the same.
> 
> I have the specs from UX in the email. Let me know if i should attach that
> to the bug. 
> 
> Thanks

All the field labels and button labels on video info screen and size units are localized.
Flags: needinfo?(skasetti)
(Assignee)

Updated

5 years ago
Attachment #794218 - Flags: ui-review?(padamczyk)
(Assignee)

Updated

5 years ago
Attachment #794218 - Flags: ui-review?(rmacdonald)
Attachment #794218 - Flags: ui-review?(padamczyk)
Attachment #794218 - Flags: review?(skasetti)
(Assignee)

Updated

5 years ago
Attachment #794218 - Flags: ui-review?(rmacdonald) → ui-review?(padamczyk)
(Assignee)

Comment 9

5 years ago
Adding Patryk and Sri for UX and Product review. Thanks
Comment on attachment 794218 [details] [review]
Pull request to display file info for videos stored in video app

Reassigning the review to John, as I won't have time to look at it for the next few days, and because John has been kind enough to volunteer to take some reviews from me :-)  (Thanks, John!)

John: Punam and I have discussed the addition of a the media_utils.js file to the shared/js/media directory, and I like the idea. She'll be using those utilities for Gallery as well.
Attachment #794218 - Flags: review?(dflanagan) → review?(johu)
Hey Punam I loaded the fix, but for some reason the device now can't read any SD cards, so I can't review the player.
Can you just post some screenshots? Thanks.
Flags: needinfo?(pdahiya)
(Assignee)

Comment 12

5 years ago
Created attachment 795669 [details]
Screen Shots of implemented video info screen, player and gallery view
Flags: needinfo?(pdahiya)
Comment on attachment 794218 [details] [review]
Pull request to display file info for videos stored in video app

This is a nice patch. Thanks for the test case that are the first test case of video app. I think most of things are correct, except the followings:
1. make code more readable js video.js
2. do not use the magic 7rem on last-child. try to use calc on height of ul to have the correct height.
3. please also change view.js to use media_utils to formatDuration.

I will give r+ with those three items are fixed.

I also put some suggestions on github about css and timezone of test case. If I am wrong, please tell me.


And final question: do we need to land 1.5x image in this patch??? How do you feel, Patryk?
Attachment #794218 - Flags: review?(johu) → review-
(Assignee)

Updated

5 years ago
Attachment #795669 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Created attachment 796164 [details]
Screen Shots of implemented video info screen, player and gallery view

Screen Shots of updated video info screen , video player and gallery view
Created attachment 796196 [details]
review.png

I think its close, but there are things off can you please look at the spec, and make sure everything is align... top issues are:
A. In Gallery view, the divider shouldn't extend to the edge of the screen.
B. In info view, the white text needs to be higher within the row
C. In info view, the button should have a dark grey background
D. In player view, the player should be flush with the divider like (>) shouldn't have a gap.
E. The scrubber should be longer.
(Assignee)

Comment 16

5 years ago
(In reply to Patryk Adamczyk [:patryk] UX from comment #15)
> Created attachment 796196 [details]
> review.png
> 
> I think its close, but there are things off can you please look at the spec,
> and make sure everything is align... top issues are:
> A. In Gallery view, the divider shouldn't extend to the edge of the screen.
> B. In info view, the white text needs to be higher within the row
will try to adjust paddings below texts to raise the content within the row

> C. In info view, the button should have a dark grey background
can you pl provide the color code of dark grey background?

> D. In player view, the player should be flush with the divider like (>)
> shouldn't have a gap.
Are you referring to the play (>),duration and slider controls at the bottom of the player view and gap/difference in combined view of attachment? 

> E. The scrubber should be longer.
By scrubber you mean , slider straight line joining the time durations in video player. Am i correct?

Thanks Patryk for your feedback, i will try to address top issues. It will help if you can answer questions posted inline.
Flags: needinfo?(padamczyk)
Patryk, 

I have another question for you. Do we need to land 1.5x image with the same bug??
(Reporter)

Updated

5 years ago
Attachment #794218 - Flags: review?(skasetti)
(In reply to Punam Dahiya from comment #16)
> (In reply to Patryk Adamczyk [:patryk] UX from comment #15)
> > Created attachment 796196 [details]
> > review.png
> > 
> > I think its close, but there are things off can you please look at the spec,
> > and make sure everything is align... top issues are:
> > A. In Gallery view, the divider shouldn't extend to the edge of the screen.

Well, this is not your problem. We have that kind of UI from the version 1.1. But since patryk mentioned it, please also change it.

> > B. In info view, the white text needs to be higher within the row
> will try to adjust paddings below texts to raise the content within the row
> 
> > C. In info view, the button should have a dark grey background
> can you pl provide the color code of dark grey background?

You should see the building block for it: http://buildingfirefoxos.com/building-blocks/action-menu.html .

If you can change your implementation based on building block, it will be better. But we don't have a simple dialog or list dialog, like android, for this case. So, don't stuck yourself in it.

> > D. In player view, the player should be flush with the divider like (>)
> > shouldn't have a gap.
> Are you referring to the play (>),duration and slider controls at the bottom
> of the player view and gap/difference in combined view of attachment? 

Well, this is not your problem, again. Please help to change it.

> > E. The scrubber should be longer.
> By scrubber you mean , slider straight line joining the time durations in
> video player. Am i correct?

I think you are right, please find the bottom right image of Patryk's review. The horizontal line from duration to total time should be longer.

Well, this is not your problem, again. Please help to change it.

> Thanks Patryk for your feedback, i will try to address top issues. It will
> help if you can answer questions posted inline.
(Assignee)

Comment 19

5 years ago
Thanks John for your feedback. I have updated pull request with your review comments in https://bugzilla.mozilla.org/show_bug.cgi?id=902995#c13. Please review.

Patryk, 

I have implemented fix of B and C from https://bugzilla.mozilla.org/show_bug.cgi?id=902995#c15 in the updated PR

I looked into A, D and E UI fixes and those needs more time for fixing and testing in both portrait and landscape mode. I would like to fix A, D and E as a separate bug and have created https://bugzilla.mozilla.org/show_bug.cgi?id=910094 for that.

Attaching the video info screen screen shot with fix of B and C from #c15. Please review. Thanks
(Assignee)

Updated

5 years ago
Attachment #794218 - Flags: review- → review?(johu)
(Assignee)

Comment 20

5 years ago
Created attachment 796449 [details]
Video Info Screen Shot with the fix of B and C UI review feedback in  #c15
Comment on attachment 794218 [details] [review]
Pull request to display file info for videos stored in video app

Punam, 

Thanks for this patch and the good test case for timezone side effect. It looks good. I give this patch r+. r=me.

But there is a small risky place that is to check the position of cutting the 'video/' from mimetype out. You should use indexOf to find the position '/' instead of hardcode the number. Although the mime type may always be video/*, we can't make sure it is always to be that in the future. Please change it before land the code.
Attachment #794218 - Flags: review?(johu) → review+
Punam, 

I don't know if you have the authority to land the code. If no, please ping me and I can help you when everything is ready (got r+ from patryk and sri).
(Assignee)

Comment 23

5 years ago
Created attachment 796816 [details]
Video Info Screen with UI fixes and Indented text content
(Assignee)

Updated

5 years ago
Attachment #796449 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
Created attachment 796836 [details]
Video-Info-Screen updated indentation and UI fixes
Attachment #796816 - Attachment is obsolete: true
Comment 24: Looks good ship it.
Flags: needinfo?(padamczyk)
Created attachment 796839 [details]
actionicon_media_info_45x45.png

Action icon @1.5x
Created attachment 796847 [details]
actionicon_media_info_45x45.png

Optimized
Attachment #796839 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
(In reply to John Hu [:johnhu] from comment #22)
> Punam, 
> 
> I don't know if you have the authority to land the code. If no, please ping
> me and I can help you when everything is ready (got r+ from patryk and sri).
Hi John
I have updated the PR with  your feedback to use indexOf to get the position of '/'.
Pull request is also updated with 1.5x image and small css fixes for UI changes from Patryk. 

We have a go from Patryk, please land the code in master. Thanks
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment #794218 - Flags: ui-review?(padamczyk)
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.