1055104 - Theme switching panel

Status: RESOLVED FIXED

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

Description

[[:fabrice] Fabrice Desré]

Olle, I'm assigning that to you since you already have a patch in bug 1011738.

Once you feel it's good to review, ask one of the peers from the settings app at https://wiki.mozilla.org/Modules/FirefoxOS to look at it.

Comment 1

[Olle Klang]

OK, I'll have a look at it!

Comment 2

[Olle Klang]

Attachment: themes.png

Comment 3

[Olle Klang]

Attachment: Screenshot settings themes panel.png

Screenshots for ui-review of the themes settings panel. Obviously the setting needs a icon but I will leave that up to someone else.

Comment 4

[Olle Klang]

Attachment: bug_1055104_theme_switching_panel.patch

Patch ready for feedback.

Comment 5

[Patryk Adamczyk [:patryk] UX]

Comment on attachment 8476561 [details]
Screenshot settings themes panel.png

Hey Olle, 
Looks solid for a first pass...

My comments:
1. The "Themes" header text should be centered
2. "Gaia" should be removed from the theme names
3. The default theme should be called "Firefox OS"
4. The other themes can just have a colour name: "Pink" "Green"

Is there a spec for this feature, it would be very useful to see a screenshot of the theme, similar to what we do with the wallpaper picker.

Comment 6

[Patryk Adamczyk [:patryk] UX]

Olle, is a designer creating a themes icon? Or do you need my team to do so?

Comment 7

[Olle Klang]

(In reply to Patryk Adamczyk [:patryk] UX from comment #6)
> Olle, is a designer creating a themes icon? Or do you need my team to do so?

Hi Patryk thanks for your feedback. Yes I would need some help with the themes icon. I would prefer if it was added to the the icons-font used for the other settings icons.

Comment 8

[Olle Klang]

(In reply to Patryk Adamczyk [:patryk] UX from comment #5)
> 1. The "Themes" header text should be centered
Will do

> 2. "Gaia" should be removed from the theme names
> 3. The default theme should be called "Firefox OS"
> 4. The other themes can just have a colour name: "Pink" "Green"
> 
> Is there a spec for this feature, it would be very useful to see a
> screenshot of the theme, similar to what we do with the wallpaper picker.

Themes will be packaged in certified theme-apps and are separated from this patch. The settings panel will fetch the name of the theme and display it in the list. I have nothing to show as they are not defined yet but Fabrice has posted some examples (for demo purposes only) in bug 1011738 to show what can be accomplished:

https://bugzilla.mozilla.org/attachment.cgi?id=8425174
https://bugzilla.mozilla.org/attachment.cgi?id=8425175

Comment 9

[Olle Klang]

Attachment: themes_panel_v2.png

Updated screenshot of themes panel for review.

Comment 10

[Olle Klang]

(In reply to Patryk Adamczyk [:patryk] UX from comment #5)

> Is there a spec for this feature, it would be very useful to see a
> screenshot of the theme, similar to what we do with the wallpaper picker.

I read your comment again and think I missed your point before. You mean a screenshot to preview the chosen theme? A theme will be applied instantaneous when you push it's checkbox which gives the user a quick look and feel of how it looks so a screenshot might be overkill.

Comment 11

[Evelyn Hung]

Comment on attachment 8476576 [details] [diff] [review]
bug_1055104_theme_switching_panel.patch

The patch looks good to me. 
Please send a pull request, so our CI process will make sure your patch doesn't break anything, then flag EJ Chen as your next round reviewer. Thanks!

Regarding submitting a gaia patch, here is a guidance:
https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Submitting_a_Gaia_patch

Comment 12

[Patryk Adamczyk [:patryk] UX]

Comment on attachment 8478867 [details]
themes_panel_v2.png

Thanks that is much better. 
Re: Thumbnails
I was thinking something like this: http://www.theberryfix.com/wp-content/uploads/QL_106390459.jpg (note this UI is over 5 years old), but as theming evolves there will be homescreen changes, downloadable themes, new wallpapers. So it would be good to create a more future proof framework right from the beginning.

Comment 13

[Patryk Adamczyk [:patryk] UX]

Helen, can you please create an icon for Olle.

Comment 14

[Helen Huang [:HHuang]]

Okay! I will provide an icon asap.

Comment 15

[Olle Klang]

Attachment: GitHub pull-request

Comment 16

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8480398 [details] [review]
GitHub pull-request

Hi Olle.klang, thanks for providing this patch ! 

Basically this patch looks nice to me, but there are something needs to change and I left all of them on github.

Please go check it and r? again when you fix them.

BTW, is there any theme app already to test with ?! Thanks !

Comment 17

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8480398 [details] [review]
GitHub pull-request

EJ has been reviewed the patch once and it seems the patch is still being revised.

Comment 18

[Olle Klang]

Attachment: GitHub pull-request

Updated pull request for review

Comment 19

[Olle Klang]

Attachment: Three-dummy-themes.patch

Three dummy themes for testing

Comment 20

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8481330 [details]
GitHub pull-request

Thanks Olle,

there are few nits on github and please check it there, and for me, I think this patch is almost done !

In addition to nits, I had one question about selected theme, is there any app or Gecko would use that to do anything after we select a theme ? Can I test what would happen when set the key ? 

Thanks for your great works !

Comment 21

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Helen, I noticed you are the visual who takes care of the icon based on previous comments. Does this icon finished already and can be used from gaia-icon ? thanks :)

Comment 22

[Helen Huang [:HHuang]]

Hi EJ, I've been designing this icon, but it needs some time for visual reviewing, probably could update it by this week. Leaving ni on myself as a reminder.

Comment 23

[Olle Klang]

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #21)
(In reply to Helen Huang from comment #22)

Actually, if the icon is put in the shared icon-font and linked via css to the 'icon-settings-themes' class (the same way all the other icons is linked), no additional changes is needed in this patch. Maybe we need to create a separate bug for updating the icon font?

Comment 24

[Olle Klang]

Attachment: GitHub pull-request

Comment 25

[Olle Klang]

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #20)
> Comment on attachment 8481330 [details]
> GitHub pull-request

> In addition to nits, I had one question about selected theme, is there any
> app or Gecko would use that to do anything after we select a theme ? Can I
> test what would happen when set the key ? 

See answer on github

Comment 26

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8482179 [details] [review]
GitHub pull-request

Hi Olle,

please make sure CI is green before merging.

Thanks for your hard works :)

And please file a follow-up depends on this bug to make sure we won't forget to update the gaia-icon.

Comment 27

[Olle Klang]

(In reply to Helen Huang from comment #22)
> Hi EJ, I've been designing this icon, but it needs some time for visual
> reviewing, probably could update it by this week. Leaving ni on myself as a
> reminder.

Hi Helen, I've posted Bug 1061133 as a followup on the icon.

Comment 28

[Helen Huang [:HHuang]]

Hi Olle, thanks for letting me know. I'll update the icons to Bug 1061133 once I done.

Comment 29

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/gaia/commit/223e149919fcd0efa4c83664f852a410e748e7f6