Closed Bug 1069791 Opened 11 years ago Closed 11 years ago

Add Firefox OS default theme

Categories

(Firefox OS Graveyard :: Gaia, defect)

Product:
Firefox OS Graveyard
Component:
Gaia
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:2.1+, b2g-v2.1 verified, b2g-v2.2 verified)

Status:
VERIFIED FIXED
Milestone:
2.1 S5 (26sep)
Project Flags:
blocking-b2g 2.1+
Tracking Flags:
Tracking Status
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: olle.klang, Assigned: olle.klang)

References

Details

Attachments

(2 files, 6 obsolete files)

GitHub pull-request
46 bytes, text/x-github-pull-request
vingtetun
: review+
fabrice
: approval-gaia-v2.1+
Details | Review
verified_v2.1.png
25.53 KB, image/png
Details
Adds a theme app with current default values. Note that this theme will not do anything until themeable apps links to the shared folder are updated.
Adds a copy of the shared/style folder to the theme.
Attachment #8492033 - Flags: review?(21)
Blocks: 1002469
Attachment #8492030 - Flags: review?(21)
Blocks: 1069840
Blocks: 1069850
Comment on attachment 8492030 [details] [diff] [review] 0001-Add-Firefox-OS-default-certified-theme.patch Review of attachment 8492030 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me but I would like Wilson to have a look at the bower bits. ::: apps/default_theme/index.html @@ +1,3 @@ > +<!DOCTYPE html> > +<html> > +<head> nit: add a <meta charset="utf-8"> just because the parser complains otherwise when this app is opened directly. I know it is not intended to be. ::: apps/default_theme/manifest.webapp @@ +1,2 @@ > +{ > + "name": "Firefox OS", s/Firefox OS/Default Theme/ (for branding purpose we avoid to use the word Firefox if we can). @@ +1,3 @@ > +{ > + "name": "Firefox OS", > + "description": "Firefox OS default theme.", s/Firefox OS default theme/Default Theme/
Attachment #8492030 - Flags: review?(wilsonpage)
Attachment #8492030 - Flags: review?(21)
Attachment #8492030 - Flags: review+
Comment on attachment 8492033 [details] [diff] [review] 0002-Adds-a-copy-of-shared-style-folder-in-default-theme.patch Review of attachment 8492033 [details] [diff] [review]: ----------------------------------------------------------------- I'm a bit uncomfortable of making a raw copy. I would like to keep the log history/blame of those files. What about using |git mv| instead ? Also I think you may need to fix the build script once this is done. As of today all those files are pulled from the theme.gaiamobile.org app for apps, but afaik there is also a local copy of them into the apps. Is it correct ? If so I feel like we should save some space by disabling this part of the build system. I would like to not remove it completely since we may re-use it partly for some of the CSS files once we switch from raw CSS to CSS variables - but instead have a flag to turn it on or off.
Attachment #8492033 - Flags: review?(21) → review-
Comment on attachment 8492030 [details] [diff] [review] 0001-Add-Firefox-OS-default-certified-theme.patch Review of attachment 8492030 [details] [diff] [review]: ----------------------------------------------------------------- I need a little more insight into how this theming solution with work. It looks as though you have copy/pasted the gaia-theme directory from shared/elements/gaia-theme/, if this is true, this is not really the best approach. If an app needs to depend on gaia-theme there are two options for installation: 1. Link to shared/gaia-theme/style.css in the <head> of your app's document and allow the Gaia build to take care of installing this asset (see other Gaia apps, like Settings for example) 2. Create a bower.json file in the root of your app's project and run `bower install gaia-components/gaia-theme` (see Camera app as an example) Please let know if you have any questions. FInd me on IRC (wilsonpage) if you want to talk it over.
blocking-b2g: --- → 2.1+
(In reply to Wilson Page [:wilsonpage] from comment #4) > Comment on attachment 8492030 [details] [diff] [review] > 0001-Add-Firefox-OS-default-certified-theme.patch > > Review of attachment 8492030 [details] [diff] [review]: > ----------------------------------------------------------------- > > I need a little more insight into how this theming solution with work. It > looks as though you have copy/pasted the gaia-theme directory from > shared/elements/gaia-theme/, if this is true, this is not really the best > approach. Hi Wilson, I'll try to recap what's going on here. The theme framework in short: Themes are contained in separate theme apps. The framework lets certified apps link to theme assets by using the following path "app://theme.gaiamobile.org/path/to/assets". The framework changes the "theme" part to the current theme and reloads the css of all open apps when a new theme is set (see bug 1011738, bug 1055104 and bug 1015853). So for a theme to change the appearance of Gaia it would have to override gaia-theme css, the assets in shared/style and provide a new wallpaper image. An ideal theme app would be more lightweight and it will be once all shared/style items has been replaced with the web components in shared/elements. A theme app would then only need to override the shared/elements/gaia-theme/style.css file and provide a wallpaper. What we intend to do with the default theme is to move the gaia-theme css and all shared/style from it's current location into a default theme app that will always be present. Obviously all themeable apps would have to update their paths to the shared folder (see bug 1069840). So the theme app does not depend on the gaia-theme it will hold it for all other apps that depend on it. It's not obvious from the patches as I copied the gaia-theme and shared/style folder instead of moving it. I'll update the patches and use git mv.
Flags: needinfo?(wilsonpage)
Attachment #8492030 - Attachment is obsolete: true
Attachment #8492030 - Flags: review?(wilsonpage)
Attachment #8492958 - Flags: review?(wilsonpage)
Assignee: nobody → olle.klang
Target Milestone: --- → 2.1 S5 (26sep)
Comment on attachment 8492958 [details] [diff] [review] 0001-Add-Firefox-OS-default-certified-theme_v2.patch Review of attachment 8492958 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Olle Klang from comment #5) > So the theme app does not depend on the gaia-theme it will hold it for all > other apps that depend on it. It's not obvious from the patches as I copied > the gaia-theme and shared/style folder instead of moving it. I'll update the > patches and use git mv. Maybe I wasn't clear, but the gaia-theme source code does not live inside the Gaia repo, it lives in its own repo [1]. We have extracted this code out of Gaia so that we can produce standalone visual style-guides, it can be versioned and other Mozilla teams (like Brick) have easy access to the source. The code you are `git mv`ing is third-party code that was installed into the repo using Bower [2]. It seems the solution you require is 'Option 2' (outlined in comment 4). If this doesn't make sense please give me a shout on IRC (:wilsonpage), and I'll be happy to walk you through it. This patch also seems to remove the existing shared/gaia-theme code which will break apps that currently depend on it. Have I missed something here? [1] http://github.com/gaia-components/gaia-theme [2] http://bower.io
Attachment #8492958 - Flags: review?(wilsonpage)
Flags: needinfo?(wilsonpage)
Attachment #8492958 - Attachment is obsolete: true
Attachment #8493679 - Flags: review?(wilsonpage)
(In reply to Wilson Page [:wilsonpage] from comment #7) > Comment on attachment 8492958 [details] [diff] [review] > 0001-Add-Firefox-OS-default-certified-theme_v2.patch > > Review of attachment 8492958 [details] [diff] [review]: > ----------------------------------------------------------------- > > (In reply to Olle Klang from comment #5) > Maybe I wasn't clear, but the gaia-theme source code does not live inside > the Gaia repo, it lives in its own repo [1]. We have extracted this code out > of Gaia so that we can produce standalone visual style-guides, it can be > versioned and other Mozilla teams (like Brick) have easy access to the > source. The code you are `git mv`ing is third-party code that was installed > into the repo using Bower [2]. > > This patch also seems to remove the existing shared/gaia-theme code which > will break apps that currently depend on it. Have I missed something here? OK, I need the shared/gaia-theme css in the default theme as all apps will link to that (bug 1069840). From your input I realize it would be unnecessary to move the files so I'll rely on the build script. When the theme is in place apps would not need to copy the gaia-theme but I guess we could break that out in a separate bug.
Attachment #8492033 - Attachment is obsolete: true
Attachment #8493685 - Flags: review?(21)
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #3) > Comment on attachment 8492033 [details] [diff] [review] > 0002-Adds-a-copy-of-shared-style-folder-in-default-theme.patch > > Review of attachment 8492033 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm a bit uncomfortable of making a raw copy. I would like to keep the log > history/blame of those files. > What about using |git mv| instead ? > > Also I think you may need to fix the build script once this is done. As of > today all those files are pulled from the theme.gaiamobile.org app for apps, > but afaik there is also a local copy of them into the apps. Is it correct ? > If so I feel like we should save some space by disabling this part of the > build system. I've updated the patch using git mv and changed the path used by the build system so shared/style resources are copied from the default theme. > I would like to not remove it completely since we may re-use it partly for > some of the CSS files once we switch from raw CSS to CSS variables - but > instead have a flag to turn it on or off. In Bug 1069840 where we redirect the apps I've added flags to disable the copying of the style files.
Flags: needinfo?(21)
Comment on attachment 8493685 [details] [diff] [review] 0002-Moves-the-shared-style-folder-to-default-theme.patch Review of attachment 8493685 [details] [diff] [review]: ----------------------------------------------------------------- r+.
Attachment #8493685 - Flags: review?(21) → review+
Comment on attachment 8493679 [details] [diff] [review] 0001-Adds-Firefox-OS-default-certified-theme.patch Looks better now. Once all app have moved over to use the theme app we should remove gaia-theme from shared/elements/ and install it directly into the default_theme app.
Attachment #8493679 - Flags: review?(wilsonpage) → review+
Attached file GitHub pull-request (obsolete) —
Keywords: checkin-needed
Flags: needinfo?(21)
Attached file GitHub pull-request —
Considering the limited time frame I suggest we go back to copying the files but depend on the build script instead as in the updated patch, and continue to work on updating the integration test in a separate bug before we can move the shared/folder.
Attachment #8493679 - Attachment is obsolete: true
Attachment #8493685 - Attachment is obsolete: true
Attachment #8494453 - Attachment is obsolete: true
Attachment #8495239 - Flags: review?(21)
Comment on attachment 8495239 [details] [review] GitHub pull-request Sounds good to me as well.
Attachment #8495239 - Flags: review?(21) → review+
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/2ffa0cdad4da4763f0b4134d70a50f030f3715d5
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
status-b2g-v2.2: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Please add a Gaia v2.1 nomination when you get a chance :)
status-b2g-v2.1: --- → affected
Flags: needinfo?(olle.klang)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19) > Please add a Gaia v2.1 nomination when you get a chance :) Pardon my ignorance Ryan, can you describe in more detail what you want me to do?
Flags: needinfo?(olle.klang)
Flags: needinfo?(ryanvm)
(In reply to Olle Klang from comment #20) > (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19) > > Please add a Gaia v2.1 nomination when you get a chance :) > > Pardon my ignorance Ryan, can you describe in more detail what you want me > to do? Click the "Details" link of your attachment, select "?" on the "approval‑gaia‑v2.1" dropdown, and fill out the questions it asks.
Flags: needinfo?(ryanvm)
Comment on attachment 8495239 [details] [review] GitHub pull-request [Approval Request Comment] [Bug caused by] (feature/regressing bug #): bug 1002469 [User impact] if declined: Bug 1069840 and Bug 1069850 can not be committed without this bug meaning users can not use themes in gaia. [Testing completed]: Yes [Risk to taking this patch] (and alternatives if risky): The patch adds a separate app which by itself impose no risk to the rest of the system. [String changes made]: -
Attachment #8495239 - Flags: approval-gaia-v2.1?
Attachment #8495239 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
This issue has been successfully verified on Flame 2.1. See attachment: verified_v2.1.png. Reproducing rate: 0/5 Flame 2.1 versions: Gaia-Rev 8ae086c39011bc8842b2a19bb5267906fa22345a Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ebbd5c65c3c1 Build-ID 20141124094013 Version 34.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141124.130744 FW-Date Mon Nov 24 13:07:55 EST 2014 Bootloader L1TC00011880
status-b2g-v2.1: fixed → verified
Depends on: 1080403
This issue has been successfully verified on Flame 2.2. Reproducing rate: 0/5 Flame 2.2 build: Gaia-Rev 824a61cccec4c69be9a86ad5cb629a1f61fa142f Gecko-Rev https://hg.mozilla.org/mozilla-central/rev/acde07cb4e4d Build-ID 20141125040209 Version 36.0a1 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141125.113029 FW-Date Tue Nov 25 11:30:43 EST 2014 Bootloader L1TC00011880
status-b2g-v2.2: fixed → verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: