gasolin@mozilla.com Reporter
	Description • 11 years ago

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

	Juwei Huang[:juwei]
	Comment 2 • 11 years ago

Hi all,
Gallery spec uploaded, thanks.

	Neo Hsieh [:neo]
	Comment 3 • 11 years ago

Photo frame UX spec

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 4 • 11 years ago

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.

	Francis Lee [:frlee]
	Comment 5 • 11 years ago

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)

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 6 • 11 years ago

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.

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 7 • 11 years ago

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.

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 8 • 11 years ago

Comment on attachment 822860 [details]
PR to master

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

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 9 • 11 years ago

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.

	Francis Lee [:frlee]
	Comment 10 • 11 years ago

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

	David Flanagan [:djf]
	Comment 11 • 11 years ago

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}]

	David Flanagan [:djf]
	Comment 12 • 11 years ago

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%;"}]

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 13 • 11 years ago

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.

	David Flanagan [:djf]
	Comment 14 • 11 years ago

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.

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 15 • 11 years ago

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?

	David Flanagan [:djf]
	Comment 17 • 11 years ago

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.

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 18 • 11 years ago

Comment on attachment 822860 [details]
PR to master

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

	John Hu [:johnhu][:johu][:醬糊小弟]
	Comment 19 • 11 years ago

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.

	Francis Lee [:frlee]
	Comment 20 • 11 years ago

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

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 21 • 11 years ago

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 ...

	John Hu [:johnhu][:johu][:醬糊小弟]
	Comment 22 • 11 years ago

Comment on attachment 822860 [details]
PR to master

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

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 23 • 11 years ago

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.

	John Hu [:johnhu][:johu][:醬糊小弟]
	Comment 24 • 11 years ago

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.

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 25 • 11 years ago

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:

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 26 • 11 years ago

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

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 27 • 11 years ago

Hi,
I record this video for your reference.

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 28 • 11 years ago

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:

	Rob MacDonald [:robmac]
	Comment 29 • 11 years ago

(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

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 31 • 11 years ago

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.

	Juwei Huang[:juwei]
	Comment 32 • 11 years ago

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

	Juwei Huang[:juwei]
	Comment 33 • 11 years ago

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

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 34 • 11 years ago

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.

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 35 • 11 years ago

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

	Juwei Huang[:juwei]
	Comment 36 • 11 years ago

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.

	John Hu [:johnhu][:johu][:醬糊小弟]
	Comment 37 • 11 years ago

Comment on attachment 822860 [details]
PR to master

Looks good.

	John Hu [:johnhu][:johu][:醬糊小弟]
	Comment 38 • 11 years ago

Comment on attachment 822860 [details]
PR to master

David,

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

	David Flanagan [:djf]
	Comment 39 • 11 years ago

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.

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 40 • 11 years ago

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.

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 41 • 11 years ago

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

	(Inactive after June) George Duan [:gduan] [:喬智] Assignee
	Comment 42 • 11 years ago

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