928254 - [Flatfish][Gallery] support 2 column layout for tablet

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect)

Description

[[e-mail]]

Make Gallery support 2 column layout for tablet.
The left panel is thumbnails list, the right panel is the fullscreen view.

Comment 2

[Juwei Huang[:juwei]]

Attachment: [Flatfish] Gallery_20131022.pdf

Hi all,
Gallery spec uploaded, thanks.

Comment 3

[Neo Hsieh [:neo]]

Attachment: Digital_Photo_Frame_131023.pdf

Photo frame UX spec

Comment 4

[(Inactive after June) George Duan [:gduan] [:喬智]]

As discussed with Mike before, we don't implement swiping function on page 13 at this stage.

(In reply to Juwei Huang from comment #2)
> Created attachment 820787 [details]
> [Flatfish] Gallery_20131022.pdf
> 
> Hi all,
> Gallery spec uploaded, thanks.

Comment 5

[Francis Lee [:frlee]]

this is a committed feature for Flatfish. according to the triage + target completed date discussion, changed to 1.3+ and set target milestones (landed code on 11/14)

Comment 6

[(Inactive after June) George Duan [:gduan] [:喬智]]

I will reuse fullscreen's mediaframe for preview, so the swiping on preview is workable.
(In reply to George Duan [:gduan] from comment #4)
> As discussed with Mike before, we don't implement swiping function on page
> 13 at this stage.
> 
> (In reply to Juwei Huang from comment #2)
> > Created attachment 820787 [details]
> > [Flatfish] Gallery_20131022.pdf
> > 
> > Hi all,
> > Gallery spec uploaded, thanks.

Comment 7

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to master

Hi David,

sorry to notice you so lately,

this patch has updated,
1. use [data-view="xxxview"] to switch different layout
2. separate the data-view styles into gallery_small.css and gallery_large.css
3. add preview view for tablet (separate two columns when landscape)
4. customize gallery_large.css to meet table's UX requirement

Since the video on gallery part is only tested on mobile, there might be or not some more changes after verify.
If possible, I would still like to ask your opinion on my patch first in general.

Thanks.

Comment 8

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 822860 [details]
PR to master

Hi Diego,
it'll be great if I can have your feedback on this patch, too.

Comment 9

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 822860 [details]
PR to master

Hi,
Things work fine on mobile as I tested, and most cases on tablet since I haven't really played video on gallery due to gecko issue.

could you kindly help to review my patch?
Thanks.

Comment 10

[Francis Lee [:frlee]]

hi David, Diago,

is it possible to review George's patch with your earliest convenient time? or please let us know when is your targeted time. this feature is still targeted to be landed by 11/5, we need your great help and support here.

thank you very much

Comment 11

[David Flanagan [:djf]]

When I apply this patch to master and run the gallery, I can see thumbnails, but when I tap on a thumbnail I get:

E/GeckoConsole(25144): [JavaScript Error: "ReferenceError: resetFrames is not defined" {file: "app://gallery.gaiamobile.org/js/gallery.js" line: 665}]

Comment 12

[David Flanagan [:djf]]

I see that resetFrames is defined in frames.js. Maybe it isn't being lazy loaded soon enough?

Also, I see this CSS error when I start the app up:  [JavaScript Warning: "Error in parsing value for 'box-shadow'.  Declaration dropped." {file: "app://gallery.gaiamobile.org/style/gallery_large.css" line: 205 column: 38 source: "  box-shadow: 0px 0px 5px 0px #000000 50%;"}]

Comment 13

[(Inactive after June) George Duan [:gduan] [:喬智]]

Thanks David,
I just push a new commit to fix the issues you've addressed as below.

1. remove doms depends on device layout, so we don't use -tablet elements anymore, 
2. remove HasFocusdOnThumbnail, instead, we use currentFrame.displayingImage,
3. lazy load frames_script.js before switching view, 
4. resize the frames when orientation change and currentView is in thumbnailListView condition

Please kindly check again.. thanks for your patience.

Comment 14

[David Flanagan [:djf]]

Comment on attachment 822860 [details]
PR to master

George,

You've done a lot of work on this bug!  I have lots of suggestions for small improvements, but overall it looks good.  Of course I don't have a tablet to test it on, so I can't really try it out.

Comment 15

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 822860 [details]
PR to master

Thanks for your patience, I push a new commit on it to fix most of issues you've addressed.
Could you kindly check it again?

Comment 17

[David Flanagan [:djf]]

I'm sorry that I still haven't made time to review this. Since John is doing a similar patch for Video, maybe he could review this patch.

Comment 18

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 822860 [details]
PR to master

Hi John,
as David suggested, could you kindly help to review my patch?
Thanks

Comment 19

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 822860 [details]
PR to master

Some problems found, told to george offline. I clear the review flag. If you fix that, please put the review again.

Comment 20

[Francis Lee [:frlee]]

implementation resource and ETA date are both agree with RD team. this has to be done before 1.3FC

Comment 21

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 822860 [details]
PR to master

Hi John,
since the gallery can play video, I've made some change my patch.

Because the video player is inside each frame, I've done some tricks for header toolbar and also the player mask background. And I also handle the rotate and transition problem. Basically, it should be just like video app.

Please kindly check and let me know your concern. Thanks.

I'll open another bug for UI test ...

Comment 22

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 822860 [details]
PR to master

I left some comment on the PR. Please find them.

Comment 23

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 822860 [details]
PR to master

Hi John,
I already address most of comment you mentioned and rebase my code to master.
Please kindly check again. Thanks.

Comment 24

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 822860 [details]
PR to master

According to offline reviewing, discussing and updating, all concerns are fixed. But please file a follow-up bug for the css overriding of building block.

If travis reports any errors about this patch, please fix that.

Comment 25

[(Inactive after June) George Duan [:gduan] [:喬智]]

Marionette test failed at below point. I think it's related to email app.

 78 passing (11 minutes)
  4 pending
  1 failing
 email notifications, disable disable notification, but still sync:

Comment 26

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: Appzip

Hi Rob,
I zip gallery app for your reference. Please kindly review it with tablet.
Thanks.

Comment 27

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: VID_0004.3gp

Hi,
I record this video for your reference.

Comment 28

[(Inactive after June) George Duan [:gduan] [:喬智]]

After rebasing to this morning's code, all tests pass.

(In reply to George Duan [:gduan] from comment #25)
> Marionette test failed at below point. I think it's related to email app.
> 
>  78 passing (11 minutes)
>   4 pending
>   1 failing
>  email notifications, disable disable notification, but still sync:

Comment 29

[Rob MacDonald [:robmac]]

(In reply to George Duan [:gduan] from comment #26)
> Created attachment 8338180 [details]
> Appzip
> 
> Hi Rob,
> I zip gallery app for your reference. Please kindly review it with tablet.
> Thanks.

Hi George...

Thanks for including me, but unfortunately I do not have a tablet to test this on. I will be in Taipei on Friday though and could try it out then if that works.

Best regards,
Rob

Comment 30

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: appzip

Comment 31

[(Inactive after June) George Duan [:gduan] [:喬智]]

Thanks Rob,

Juwei just helped us to do the first review and found some issues have/haven't listed in UX spec.

Hi Juwei,
could you kindly update?
thanks.

Comment 32

[Juwei Huang[:juwei]]

Attachment: [Flatfish] Gallery_20131127.pdf

Hi George! UX spec updated: detail of selection mode added.

Comment 33

[Juwei Huang[:juwei]]

Hi George,
Here's the bug fixed:
- dialog : top shadow of action bar should not exist - >fixed
- dialog : update background as color instead of image ->  fixed              
- progress bar when save image  -> fixed
- save image, the date should be updated -> gecko issue
- select, should show the last select -> fixed, ux spec updated.
** Always show the last selection in edit mode
** De-select a photo in edit mode, the right pane will preview previous selection
** Once users delete photos in edit mode, the right will not highlight any photo afterward. Yet when users go back to main view directly without select any photo, the first photo will highlight.
- sometimes, suddenly video cannot enlarge -> fixed
- when switch to video app in portrait, the screen rotate to landscape and when switch back, the screen rotate to portrait again, and it’s not smooth -> system & perf
- pick, empty screen while cropping -> fixed
- fullscreen button image not correct -> fixed
- pick button style -> fixed

Comment 34

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 822860 [details]
PR to master

Hi John,
I update my patch based on comment 33. Could you kindly help to check it again?
Then we can move to QA test . Thanks.

Comment 35

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: Appzip

Hi Juwei,
as Mike said, please kindly help to review the UI of gallery app on tablet.
Thanks.

Comment 36

[Juwei Huang[:juwei]]

Comment on attachment 8340195 [details]
Appzip

Review done! Thanks George:)
* Delete flow modified: Once photos deleted in edit mode, highlight the photo which located next to the latest deleted photo when go back main mode.

Comment 37

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 822860 [details]
PR to master

Looks good.

Comment 38

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 822860 [details]
PR to master

David,

According to the email, please give this patch an overall review.

Comment 39

[David Flanagan [:djf]]

Comment on attachment 822860 [details]
PR to master

I don't have time to do a thorough review, and I know you really want to land this.  I've left some comments on github.

I'm concerned about the visual design for the tablet diverging from the visual refresh that the visdev team wants to apply. This patch is going to make that visual refresh much harder to achieve.  

It doesn't seem like changing  apps/gallery/style/images/actionicon_gallery_exposure.png right now is a good idea since there are pending visual changes for all the icons.

I'm also surprised at all the images in this PR given the new visual design focus on "made with code". I thought we were going to minimize images in new designs.

I know that you really have to land this patch. Please coordinate with Pavel or whoever is working on the visual changes to gallery.

Comment 40

[(Inactive after June) George Duan [:gduan] [:喬智]]

Thanks David.
I just discussed with taipei visual team, and made some changes, including use actionicon_gallery_edit_exposure_30x30.png and use background color instead of image.

Comment 41

[(Inactive after June) George Duan [:gduan] [:喬智]]

We had discussed with tablet visual member about mobile visual changes. She will track the changes that may affect the tablet visual.

Land to master,
https://github.com/mozilla-b2g/gaia/commit/8521e1f89e55b5b2508a0bcec878e3505b1e800b

Comment 42

[(Inactive after June) George Duan [:gduan] [:喬智]]

The delete dialog is empty.
I open bug 949966 to fix it.