Closed Bug 1146541 Opened 5 years ago Closed 5 years ago

Merge autothemer back to master branch


(Firefox OS Graveyard :: Gaia::Theme Editor, defect, P1)



(Not tracked)



(Reporter: drs, Assigned: julienw)




(Whiteboard: [spark])


(2 files)

Julien made some changes to the Theme Editor to support pulling colors out of pictures from the Gallery or taken using the camera, but it's sitting on his own branch.

We should get this (UI-)reviewed and merged back to the master branch.
Attached file Demo video
Jacqueline, I made a video of a quick look at the changes in this branch. It will be helpful for you when working on the spec.
Flags: needinfo?(jsavory)
From the video, it looks like the "autotheme"  does not work, but it really should :) Maybe it doesn't work from where you triggered it.

Here are 2 additional videos:
Attached file github PR
Just an untested rebased version
Works as intended in Firefox.

(same profile as the test-agent ("DEBUG=1 DESKTOP=0 make"), run "python -mSimpleHTTPServer" in the directory, access http://localhost:8000)
Needs the patch from bug 1111961 to be able to install the theme.
Depends on: 1111961
Comment on attachment 8584686 [details] [review]
github PR

Here is a commented PR :)

Remember you need a gecko with the patch from bug 1111961 so that installing a theme works !

There is some known bugs that I didn't have the time to debug:
* sometimes, in the "create theme" dialog, I have the "clear autotheme" cross even when no autotheme palette is displayed.
* sometimes, creating a theme doesn't create it.

I think we can try to fix theme in follow-up bugs.
Attachment #8584686 - Flags: review?(etienne)
Comment on attachment 8584686 [details] [review]
github PR

Here we go, some small comments on github, and lots of :+1:! This is a great improvement to the app.

But I want to discuss two bigger points here:

* until now I tried to keep the app bare-minimum "external libs"-wise, except for gaia-components (which are heavily used in other lightsaber apps).
I'm not completely opposed to adding dependencies, but if we do it, it should make the app *more* like other lightsaber apps, not diverge further. To put it a bit bluntly studio should become more like the sharing app with time [1] not more like the sms app :/
There's room for compromise :
|defer.js| is small enough, but I'm not so sure about introducing |event_dispatcher.js| as part of an Auto Theme patch. CustomEvents on the window should perfectly do the trick for now since you're only emitting from singleton objects anyway.

* the global state is probably going to cause trouble down the line :) But I'm fine with just filing a follow up for now.

Attachment #8584686 - Flags: review?(etienne)
Thanks for the videos guys, it was very helpful when creating the spec!
Flags: needinfo?(jsavory)
Whiteboard: [lightsaber] → [ignite]
I fixed Etienne's comments + other bugs, but I found that the activity was not working well if the app was previously running. So I want to investigate this before asking a new review.
Whiteboard: [ignite] → [spark]
Component: Gaia → Gaia::Theme Editor
Priority: P2 → P1
Comment on attachment 8584686 [details] [review]
github PR

This time I think it's ready.

The activity issue was caused by bug 818000 (I hate this bug), I worked around it by removing the hash in the activity entry point.

Please test properly on a cypress branch as I don't have one handy :/

Thanks !
Attachment #8584686 - Flags: review?(etienne)
(tested on cypress too)
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #8584686 - Flags: review?(etienne) → review+
You need to log in before you can comment on or make changes to this bug.