Closed Bug 1146541 Opened 5 years ago Closed 5 years ago
Merge autothemer back to master branch
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.
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.
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: http://people.mozilla.org/~jwajsberg/fxos/v3/autotheme-1.webm http://people.mozilla.org/~jwajsberg/fxos/v3/autotheme-2.webm
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)
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.
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  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 AutoTheme.active is probably going to cause trouble down the line :) But I'm fine with just filing a follow up for now.  https://github.com/fxos/sharing
Thanks for the videos guys, it was very helpful when creating the spec!
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.
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 !
(tested on cypress too) https://github.com/fxos/studio/commit/d10b7864ad0bb1f185ce1349da9be03b861badcf
Status: NEW → RESOLVED
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.