1000132 - [Building Blocks] Split headers.css to demo @import

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Comment 1

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Let's initially follow the structure we had in:
https://github.com/rnowm/Gaia-UI-Building-Blocks/tree/v2/style/headers

Comment 2

[[e-mail]]

We have an experiment tool called purecss https://github.com/gasolin/purecss

a myth fork with @import inlining, css variable replacing, and @media merging(TBD).

george is going to write the command line tool that accept options, so our build system could use it when parsing the css from index.html.

chens propose to parse @media and overwrite/extend existing style definition, so we could send options in build time and save @media usage to improve load_time for different form factor devices.

Comment 3

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Hey Fred,
I'll play with that.
BTW: there's already a project called "purecss" any chance you rename it?
I'm not gonna suggest any name ;)

Comment 4

[[e-mail]]

Ooh we should rename it, no idea now :o

Comment 5

[[e-mail]]

ok, let's try https://github.com/gasolin/provecss

Comment 6

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

f+ to the new name ;)

Comment 7

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Attachment: patch in github

Comment 8

[[e-mail]]

precompiled result is in https://gist.github.com/gasolin/90870b1c9ac9df2aef4f

The precompile instruction is in PR.

Comment 9

[[e-mail]]

Comment on attachment 8414440 [details] [review]
patch in github

looks good to me.

From precompiler side we have a PR that can pass ['default', 'dark'] as option to not include comms or non-used skins and form factor. 

https://github.com/gasolin/provecss/pull/4


But currently the naming convention would be only support syntax like `skin_default.css` instead of `skin.default.css` to do the filter thing.

Comment 10

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Fred,
I have renamed skin files to use "_" as you suggested.
Should we add any code to that PR to inline imports or that should be done in a different PR?
Thanks!

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Arnau March  [:arnau] from comment #10)
> Fred,
> I have renamed skin files to use "_" as you suggested.
> Should we add any code to that PR to inline imports or that should be done
> in a different PR?
> Thanks!

Could be done in a followup imo.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8414440 [details] [review]
patch in github

Arnau, to me all the icons, colors informations in the layout file are pure theming information. Should be part of a skin_default.css something to me, so we would be able to overidde it in the future with bug 1002469.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #12)
> Comment on attachment 8414440 [details] [review]
> patch in github
> 
> Arnau, to me all the icons, colors informations in the layout file are pure
> theming information. Should be part of a skin_default.css something to me,
> so we would be able to overidde it in the future with bug 1002469.

That said I don't think we should block on that comment.

Comment 14

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Vivien, one question then:
If I have in default css: h1{ color:#fff }
should I have:

skin organic: h1{ color:#868692 }
skin dark: h1{ color:#fff }
skin comms: h1{ color:#fff }

Or only override when if different that default? (in that case only in skin organic)

That was my main concern for defining defaults inside layout.css
Thanks!

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Arnau March  [:arnau] from comment #14)
> Vivien, one question then:
> If I have in default css: h1{ color:#fff }
> should I have:
> 
> skin organic: h1{ color:#868692 }
> skin dark: h1{ color:#fff }
> skin comms: h1{ color:#fff }
> 
> Or only override when if different that default? (in that case only in skin
> organic)
> 
> That was my main concern for defining defaults inside layout.css
> Thanks!

I think I would just override if necessary.

Comment 16

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

I've updated my PR with your proposal :) Could you please check it again?

Comment 17

[[e-mail]]

Now provecss come with basic command-line interface. It's more easier to try and tuning the result now

$ npm install -g provecss
$ provecss headers.css headers.out.css --import

We'd going to evaluate applying provecss on settings/video app first to solve @media load_time in 1.4+. Then we'll take a look on how to integrate the build process with BB.

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Arnau March  [:arnau] from comment #16)
> I've updated my PR with your proposal :) Could you please check it again?

It looks good overall. I kind of wonder, is it not the role of the app to select the skin it wants ?

So instead of having in the global import file:

@import url("headers/skin_comms.css");
@import url("headers/skin_dark.css");
@import url("headers/skin_organic.css");

Can we let the app pick up the right theme ?

Comment 19

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

I Think that's what Fred wants to do in build time: pick which files are imported depending on each app, not to load unneeded ones. Isn't it?
Should we declare that on the manifest?

Please note that not only headers component use skins, and they could be multiple (e.g contacts has a settings section, which uses organic skin)

Comment 20

[[e-mail]]

After learn more about our build script black magic, the build optimization process the build script  already have import inlining feature(!), and will automatically concatenate css files into single file.

It means we can use import inlining now without any build system patch.
If we only include `skin.default.css` in `header.css` and keep the rest skin separately. <link> extra skin in html when needed. 

Then we could fire another issue to add import filter function, which only do the filtering for different layout (phone, tablet) type when there are provided.
That approach will make this issue not be blocked by build system patch.

Comment 21

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

(In reply to Fred Lin [:gasolin] from comment #20)
> After learn more about our build script black magic, the build optimization
> process the build script  already have import inlining feature(!), and will
> automatically concatenate css files into single file.
> 
> It means we can use import inlining now without any build system patch.
Good, I'll test that!
> If we only include `skin.default.css` in `header.css` and keep the rest skin
> separately. <link> extra skin in html when needed. 
> 
> Then we could fire another issue to add import filter function, which only
> do the filtering for different layout (phone, tablet) type when there are
> provided.
> That approach will make this issue not be blocked by build system patch.
On the other hand, it could be tricky to land, e.g:
Comms apps will now use custom building blocks for: headers (green + grey for settings), scrolling and lists
They all get applied just by using .skin-comms in the HTML.
The overridden properties for skinning are not that much, so I would prefer at least for this PR, while we figure out a better way to use themes, import all skins in all apps.
That way I don't need to go to all apps and add the extra links.
Does that make sense?

Comment 22

[Wilson Page [:wilsonpage]]

gasolin: Can you clarify how this provecss step will integrate into the Gaia build process? Do we get all the others features of Rework and Myth.io too? I'm concerned about introducing a Mozilla owner CSS pre-compiler when there are many other great, well supported/established solutions already available.

(sorry if I'm a little late to the conversation on this)

Comment 23

[[e-mail]]

Wilson, it's still the right time to express your concern.

After discussion with yuren (gaia build script owner), we don't integrate provecss into the Gaia build process in current stage. Because Gaia's main build process should be run on xpcshell and does not accept node-based command yet.

Current approach we take will be:
1. porting some most requested features (such as import filtering) into build web-optimize process to support different layout type (phone, tablet) when they are provided. 
2. These features are opt-in (off by default). Each app may define explicitly in their Makefile to enable some preferred features.

We did not tend to write yet another pre-processor at all. Myth.io is based on Rework plugins and most likely the way that help us write pure future-proved CSS. But their features-set may not fit gaia webapp need. (ex: auto vendor prefixing, css variable replacing, and Myth did not provide pref-off option)

`provecss` is based on Rework plugins and follow Myth.io's concept to quickly prove our proposals work or not.

* We assembe existing Rework plugins and wrote some custom plugins, because some approaches we proposed(inline filtering, media matching and extracting) are only suit for pre-bundled webapp (since we already know device's form factor and screen size, we could optimize for it). 
* All feature is opt-in by developer.

Comment 24

[Casey Yee]

Comment on attachment 8414440 [details] [review]
patch in github

Looks fine provided comments are addressed in PR.

Comment 25

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Arnau March  [:arnau] from comment #21)
> (In reply to Fred Lin [:gasolin] from comment #20)
> > After learn more about our build script black magic, the build optimization
> > process the build script  already have import inlining feature(!), and will
> > automatically concatenate css files into single file.
> > 
> > It means we can use import inlining now without any build system patch.
> Good, I'll test that!
> > If we only include `skin.default.css` in `header.css` and keep the rest skin
> > separately. <link> extra skin in html when needed. 
> > 
> > Then we could fire another issue to add import filter function, which only
> > do the filtering for different layout (phone, tablet) type when there are
> > provided.
> > That approach will make this issue not be blocked by build system patch.
> On the other hand, it could be tricky to land, e.g:
> Comms apps will now use custom building blocks for: headers (green + grey
> for settings), scrolling and lists
> They all get applied just by using .skin-comms in the HTML.
> The overridden properties for skinning are not that much, so I would prefer
> at least for this PR, while we figure out a better way to use themes, import
> all skins in all apps.
> That way I don't need to go to all apps and add the extra links.
> Does that make sense?

I would be happy to load less CSS if we can but it can be done in a followup. No reason to block this work as the situation is not worse than before.

Comment 26

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Merged: 8245fc78a915c83f0d71f111363005a2d3c99951