Closed Bug 928254 Opened 7 years ago Closed 7 years ago
[Flatfish][Gallery] support 2 column layout for tablet
2.80 MB, application/pdf
471.02 KB, application/pdf
358 bytes, text/html
8.90 MB, video/3gpp
2.87 MB, application/pdf
1.11 MB, application/zip
Make Gallery support 2 column layout for tablet. The left panel is thumbnails list, the right panel is the fullscreen view.
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.
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 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.
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
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.
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+]
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 on attachment 822860 [details] PR to master I left some comment on the PR. Please find them.
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 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.
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:
Hi Rob, I zip gallery app for your reference. Please kindly review it with tablet. Thanks.
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
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.
Hi George! UX spec updated: detail of selection mode added.
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)
Hi Juwei, as Mike said, please kindly help to review the UI of gallery app on tablet. Thanks.
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.
Comment on attachment 822860 [details] PR to master David, According to the email, please give this patch an overall review.
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: 7 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.