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

RESOLVED FIXED in 1.3 Sprint 4 - 11/8

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gasolin, Assigned: gduan)

Tracking

unspecified
1.3 Sprint 4 - 11/8
Other
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Flatfish only][developer+])

Attachments

(6 attachments, 2 obsolete attachments)

Make Gallery support 2 column layout for tablet.
The left panel is thumbnails list, the right panel is the fullscreen view.
Assignee: nobody → gduan
Blocks: flatfish
Duplicate of this bug: 922459
blocking-b2g: --- → 1.3?
Hi all,
Gallery spec uploaded, thanks.
Photo frame UX spec
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.
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)
blocking-b2g: 1.3? → 1.3+
Whiteboard: [Flatfish only]
Target Milestone: --- → 1.3 Sprint 4 - 11/8
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.
Posted file 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.
Attachment #822860 - Flags: review?
Attachment #822860 - Flags: feedback?(dflanagan)
Comment on attachment 822860 [details]
PR to master

Hi Diego,
it'll be great if I can have your feedback on this patch, too.
Attachment #822860 - Flags: review? → feedback?(dmarcos)
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.
Attachment #822860 - Flags: review?(dmarcos)
Attachment #822860 - Flags: review?(dflanagan)
Attachment #822860 - Flags: feedback?(dmarcos)
Attachment #822860 - Flags: feedback?(dflanagan)
Attachment #822860 - Flags: feedback-
Blocks: 929875
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
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}]
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%;"}]
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 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.
Attachment #822860 - Flags: review?(dflanagan) → review-
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?
Attachment #822860 - Flags: review?(dmarcos) → review?(dflanagan)
Duplicate of this bug: 929953
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 on attachment 822860 [details]
PR to master

Hi John,
as David suggested, could you kindly help to review my patch?
Thanks
Attachment #822860 - Flags: review- → review?(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.
Attachment #822860 - Flags: review?(johu)
implementation resource and ETA date are both agree with RD team. this has to be done before 1.3FC
Whiteboard: [Flatfish only] → [Flatfish only][developer+]
blocking-b2g: 1.3+ → ---
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 ...
Attachment #822860 - Flags: review?(johu)
Attachment #822860 - Flags: review?(dflanagan)
Attachment #822860 - Flags: review-
Comment on attachment 822860 [details]
PR to master

I left some comment on the PR. Please find them.
Attachment #822860 - Flags: review?(johu)
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.
Attachment #822860 - Flags: review?(johu)
Attachment #822860 - Flags: feedback-
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.
Attachment #822860 - Flags: review?(johu) → review+
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:
Posted file Appzip (obsolete) —
Hi Rob,
I zip gallery app for your reference. Please kindly review it with tablet.
Thanks.
Attachment #8338180 - Flags: ui-review?(rmacdonald)
Posted video VID_0004.3gp
Hi,
I record this video for your reference.
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:
(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
Posted file appzip (obsolete) —
Attachment #8338180 - Attachment is obsolete: true
Attachment #8338180 - Flags: ui-review?(rmacdonald)
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.
Flags: needinfo?(jhuang)
Hi George! UX spec updated: detail of selection mode added.
Flags: needinfo?(jhuang)
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 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.
Attachment #822860 - Flags: review+ → review?(johu)
Posted file Appzip
Hi Juwei,
as Mike said, please kindly help to review the UI of gallery app on tablet.
Thanks.
Attachment #8339043 - Attachment is obsolete: true
Attachment #8340195 - Flags: review?(jhuang)
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.
Attachment #8340195 - Flags: review?(jhuang) → review+
Comment on attachment 822860 [details]
PR to master

Looks good.
Attachment #822860 - Flags: review?(johu) → review+
Comment on attachment 822860 [details]
PR to master

David,

According to the email, please give this patch an overall review.
Attachment #822860 - Flags: review?(dflanagan)
No longer blocks: 903920
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.
Attachment #822860 - Flags: review?(dflanagan) → review+
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.
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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
The delete dialog is empty.
I open bug 949966 to fix it.
You need to log in before you can comment on or make changes to this bug.