1050404 - [Collections] Update to use gaia-header

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Everything.me, defect)

Comment 1

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 2

[Wilson Page [:wilsonpage]]

I'm using the 'dark/media' theme until I get confirmation back from visual on what theme should be used for collections app. NI hung for more info here.

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8469454 [details] [review]
pull-request (master)

I haven't tested this or anything yet, but seems like it should be fine. Thanks!

Comment 4

[Hung Nguyen]

Yes continue using the "media" theme for the dark applications. Also after speaking to Wilson, I think we should rename the "media theme" to "dark theme" to minimize confusion for any apps that want to use it.

Comment 5

[Wilson Page [:wilsonpage]]

Comment on attachment 8469454 [details] [review]
pull-request (master)

I have the commit stacked on blocker bug 1050801 purely for review purposes. Will rebase once that lands.

Comment 6

[Kevin Grandon :kgrandon]

Comment on attachment 8469454 [details] [review]
pull-request (master)

I can't R+ this because it appears to break the wallpaper that is supposed to be seen behind the header. If this is a UX change, that's fine, but we have a bunch of wallpaper code in the app that we should remove.

Comment 7

[Wilson Page [:wilsonpage]]

(In reply to Kevin Grandon :kgrandon from comment #6)
> Comment on attachment 8469454 [details] [review]
> pull-request (master)
> 
> I can't R+ this because it appears to break the wallpaper that is supposed
> to be seen behind the header. If this is a UX change, that's fine, but we
> have a bunch of wallpaper code in the app that we should remove.

kgrandon: see comment 4 where Hung states that we should use the 'dark'/'media' theme. This results in a solid grey header background.

Comment 8

[Hung Nguyen]

Sorry guys but can I get a screenshot of what's happening here? 

Thanks!

Comment 9

[Wilson Page [:wilsonpage]]

Attachment: 1015247.png

Screenshot after with new header

Comment 10

[Hung Nguyen]

Comment on attachment 8470864 [details]
1015247.png

Looks good to me.

Comment 11

[Kevin Grandon :kgrandon]

Attachment: Collections - New header.png

The typical smart collections view is with a background image. Let's make sure we ui-review the proper image. The code does look good to me though, just want to make sure we are doing the right thing UX wise.

Flagging both Hung and Patryk for ui-review, because I believe Patryk did quite a bit of work on smart collections in the past.

Comment 12

[Amir Nissim (:amirn)]

If indeed there will no wallpaper behind the header, let's please remove this code:
https://github.com/mozilla-b2g/gaia/blob/master/apps/collection/js/view_collection.js#L36

and close bug 1020957

Comment 13

[Patryk Adamczyk [:patryk] UX]

Comment on attachment 8471158 [details]
Collections - New header.png

Loaded the patch and it looks good, one thing I would change is the header colour, on the flame it feels way too bright can we use #333333 instead for these collections? Thanks!

Comment 14

[Kevin Grandon :kgrandon]

Comment on attachment 8469454 [details] [review]
pull-request (master)

Sounds good to me then. Thanks Wilson!

Comment 15

[Wilson Page [:wilsonpage]]

Comment on attachment 8469454 [details] [review]
pull-request (master)

LANDED

https://github.com/mozilla-b2g/gaia/commit/dfaecc8154daa4edaa631934264e9848e213d091