Closed Bug 1055104 Opened 5 years ago Closed 5 years ago

Theme switching panel

Categories

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

All
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S3 (29aug)

People

(Reporter: fabrice, Assigned: olle.klang)

References

Details

Attachments

(4 files, 4 obsolete files)

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.
OK, I'll have a look at it!
Attached image themes.png
Attached image Screenshot settings themes panel.png (obsolete) —
Screenshots for ui-review of the themes settings panel. Obviously the setting needs a icon but I will leave that up to someone else.
Attachment #8476561 - Flags: ui-review?(padamczyk)
Patch ready for feedback.
Attachment #8476576 - Flags: review?(ehung)
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.
Attachment #8476561 - Flags: ui-review?(padamczyk) → ui-review-
Olle, is a designer creating a themes icon? Or do you need my team to do so?
(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.
(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
Attached image themes_panel_v2.png
Updated screenshot of themes panel for review.
Attachment #8476561 - Attachment is obsolete: true
Attachment #8478867 - Flags: ui-review?(padamczyk)
(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 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
Attachment #8476576 - Flags: review?(ehung) → feedback+
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.
Attachment #8478867 - Flags: ui-review?(padamczyk) → ui-review+
Helen, can you please create an icon for Olle.
Flags: needinfo?(hhuang)
Okay! I will provide an icon asap.
Flags: needinfo?(hhuang)
Attached file GitHub pull-request (obsolete) —
Attachment #8476576 - Attachment is obsolete: true
Attachment #8480398 - Flags: review?(ejchen)
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 !
Attachment #8480398 - Flags: review?(ejchen)
Attachment #8480398 - Flags: review?(ehung)
Attachment #8480398 - Flags: review?(arthur.chen)
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.
Attachment #8480398 - Flags: review?(ehung)
Attachment #8480398 - Flags: review?(arthur.chen)
Attached file GitHub pull-request (obsolete) —
Updated pull request for review
Attachment #8480398 - Attachment is obsolete: true
Attachment #8481330 - Flags: review?(ejchen)
Three dummy themes for testing
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 !
Attachment #8481330 - Flags: review?(ejchen)
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 :)
Flags: needinfo?(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.
(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?
Attached file GitHub pull-request
Attachment #8481330 - Attachment is obsolete: true
Attachment #8482179 - Flags: review?(ejchen)
(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 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.
Attachment #8482179 - Flags: review?(ejchen) → review+
Blocks: 1061133
(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.
Keywords: checkin-needed
Hi Olle, thanks for letting me know. I'll update the icons to Bug 1061133 once I done.
Flags: needinfo?(hhuang)
master: https://github.com/mozilla-b2g/gaia/commit/223e149919fcd0efa4c83664f852a410e748e7f6
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
You need to log in before you can comment on or make changes to this bug.