Issue 5723 - Implement new abp.org content style guide

Issue 5723 - Implement new abp.org content style guide

[juliandoucette, 2017-10-07 15:37:25]

Ready for review. I think you should address this before header and footer if
you have not already started them.

I decided to add defaults.css to this Patchset instead of adding the rest of
website-defaults into main.css selectively. I made two customizations to
default.css [I changed the accent color to red, I changed the desktop breakpoint
to 1200 and 1170 width].

Adding defaults.css does 2 new big things [it adds a CSS reset, it adds .content
styles]. We should make sure that these do not break existing pages.

[juliandoucette, 2017-10-07 15:38:15]

If you think that this looks good then I will rebase away website-defaults from
main.css in the header and footer and sync the font-sizes.

[juliandoucette, 2017-10-07 15:44:37]

Note: 

I didn't set the html font-size. But I did set the heading sizes relative to the
body size specified in the styleguide. e.g. if body = 20px and h5 = 22px then h5
= ~0.9em. This allows us to specify a body size less than 20px (which is huge)
and keep the same relative sizing.

I didn't yet implement mobile sizes as specified in the styleguide. This is
because it would be a lot easier to just change the base font-size on smaller
screens than to change every font-size as specified. I think we should wait and
see how it looks after more of the design is implemented before deciding.

[juliandoucette, 2017-10-07 15:50:08]

Added notes.

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/index-mo...
File static/css/index-mobile.css (left):

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/index-mo...
static/css/index-mobile.css:4: margin-left: 0;
This prevented the mobile content from centering.

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/index.cs...
static/css/index.css:1: #key-facts h1
#key-facts headings were the only headings that I could find that this applied
to.

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/index.cs...
static/css/index.css:507: #key-facts
border-box breaks this layout because it relies on the space provided by the
padding.

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/main.css...
static/css/main.css:289: margin: 30px 0px;
(fallback in-case rem is not supported - this size may change based on the base
font-size we decide on. The default font-size is usually 30px - so I probably
should have made this 32px)

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/main.css...
static/css/main.css:299: .content footer,
I could have excluded this from this review as it only really applies to the
footer (which includes this style or similar already).

[ire, 2017-10-09 11:35:50]

On 2017/10/07 15:37:25, juliandoucette wrote:
> Ready for review. I think you should address this before header and footer if
> you have not already started them.

Oops, just saw this :/ Anyway, I'm reviewing this now.

[ire, 2017-10-09 12:28:31]

On 2017/10/07 15:44:37, juliandoucette wrote:
> Note: 
> 
> I didn't set the html font-size. But I did set the heading sizes relative to
the
> body size specified in the styleguide. e.g. if body = 20px and h5 = 22px then
h5
> = ~0.9em. This allows us to specify a body size less than 20px (which is huge)
> and keep the same relative sizing.
> 
> I didn't yet implement mobile sizes as specified in the styleguide. This is
> because it would be a lot easier to just change the base font-size on smaller
> screens than to change every font-size as specified. I think we should wait
and
> see how it looks after more of the design is implemented before deciding.

Okay.

[ire, 2017-10-09 12:28:49]

Thanks. Comments below.

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/defaults...
File static/css/defaults.css (right):

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/defaults...
static/css/defaults.css:16: * along with website-defaults.  If not, see
<http://www.gnu.org/licenses/>.
The repeated licence headers in this file should probably be removed. 

Related - I think the licence headers for partial files in website-defaults
should be done with `//` comments so they don't show up on the generated CSS.
The regular comment can exist on the main file that imports all the other files.

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/defaults...
static/css/defaults.css:36: * Variables
Again, the comments for the variables file in particular should be `//` style so
they don't show up on any generated CSS files.

(We should created a separate issue for this of course)

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/defaults...
static/css/defaults.css:102: tr, th, td {
Does our coding style guide apply to "generated" CSS files? Does this file count
as "generated"?

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/index-mo...
File static/css/index-mobile.css (left):

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/index-mo...
static/css/index-mobile.css:4: margin-left: 0;
On 2017/10/07 15:50:07, juliandoucette wrote:
> This prevented the mobile content from centering.

Acknowledged.

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/index.cs...
static/css/index.css:1: #key-facts h1
On 2017/10/07 15:50:07, juliandoucette wrote:
> #key-facts headings were the only headings that I could find that this applied
> to.

Ack. I couldn't find any other cases either.

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/index.cs...
static/css/index.css:507: #key-facts
On 2017/10/07 15:50:07, juliandoucette wrote:
> border-box breaks this layout because it relies on the space provided by the
> padding.

Acknowledged.

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/main.css...
static/css/main.css:286: .content h1
NIT (feel free to ignore if you don't agree): I think the selectors should be
ordered in descending order (i.e. h1 first)

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/main.css...
static/css/main.css:289: margin: 30px 0px;
On 2017/10/07 15:50:07, juliandoucette wrote:
> (fallback in-case rem is not supported - this size may change based on the
base
> font-size we decide on. The default font-size is usually 30px - so I probably
> should have made this 32px)

Acknowledged.

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/main.css...
static/css/main.css:293: .content h5 {font-size: 1.1em;}
There should be spaces between the "{" and the style inside

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/main.css...
static/css/main.css:299: .content footer,
On 2017/10/07 15:50:08, juliandoucette wrote:
> I could have excluded this from this review as it only really applies to the
> footer (which includes this style or similar already).

I think it should only be applied to the #site-footer element. We may want to
use the footer element elsewhere and not necessarily have this font-size
applied.

[juliandoucette, 2017-10-09 23:28:54]

New PatchSet :)

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/defaults...
File static/css/defaults.css (right):

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/defaults...
static/css/defaults.css:16: * along with website-defaults.  If not, see
<http://www.gnu.org/licenses/>.
On 2017/10/09 12:28:48, ire wrote:
> The repeated licence headers in this file should probably be removed. 
> 
> Related - I think the licence headers for partial files in website-defaults
> should be done with `//` comments so they don't show up on the generated CSS.
> The regular comment can exist on the main file that imports all the other
files.
> 

Agreed.

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/defaults...
static/css/defaults.css:102: tr, th, td {
On 2017/10/09 12:28:48, ire wrote:
> Does our coding style guide apply to "generated" CSS files? Does this file
count
> as "generated"?

No. Yes. 

Or, at least, this can be addressed separately.

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/main.css...
static/css/main.css:286: .content h1
On 2017/10/09 12:28:48, ire wrote:
> NIT (feel free to ignore if you don't agree): I think the selectors should be
> ordered in descending order (i.e. h1 first)

I agree :D

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/main.css...
static/css/main.css:293: .content h5 {font-size: 1.1em;}
On 2017/10/09 12:28:48, ire wrote:
> There should be spaces between the "{" and the style inside

Acknowledged. (Doh)

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/main.css...
static/css/main.css:299: .content footer,
On 2017/10/09 12:28:48, ire wrote:
> I think it should only be applied to the #site-footer element. We may want to
> use the footer element elsewhere and not necessarily have this font-size
> applied.

Agreed. The styleguide does specify this. But the design doesn't include many
<footer>s with smaller text; anyway. I should have known better.

[ire, 2017-10-10 07:48:44]

LGTM

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/defaults...
File static/css/defaults.css (right):

https://codereview.adblockplus.org/29568555/diff/29568556/static/css/defaults...
static/css/defaults.css:102: tr, th, td {
On 2017/10/09 23:28:53, juliandoucette wrote:
> On 2017/10/09 12:28:48, ire wrote:
> > Does our coding style guide apply to "generated" CSS files? Does this file
> count
> > as "generated"?
> 
> No. Yes. 
> 
> Or, at least, this can be addressed separately.

Alright :)

[juliandoucette, 2017-10-10 10:59:42]

Issue 5723 - Implement new abp.org content style guide

[juliandoucette, 2017-10-10 11:00:03]

Oops :D

https://bitbucket.org/adblockplus/adblockplus.org/commits/04acf9c57b9f2a145ee...