Issue 29474565: Issue 5350 - Setup Help Center Boilerplate

Issue 29474565: Issue 5350 - Setup Help Center Boilerplate (Closed)

Created:
June 26, 2017, 10:52 a.m. by ire

Modified:
July 7, 2017, 9:38 a.m.

Reviewers:
juliandoucette

Visibility:
Public.

Description

Issue 5350 - Setup Help Center Boilerplate

Patch Set 2 : Add website defaults #

Total comments: 19

Patch Set 3 : Move browserlist to package.json, Fix font urls, Add blocks in template #

Patch Set 4 : Update colour variable names and Font stack #

Patch Set 5 : Diff #

Created: July 6, 2017, 7:34 a.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1638 lines, -0 lines)			Patch
A	.gitignore	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
A	.hgignore	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
A	README.md	View	1 2 3 4	1 chunk	+9 lines, -0 lines	0 comments	Download
A	gulpfile.js	View	2 3 4	1 chunk	+37 lines, -0 lines	0 comments	Download
A	package.json	View	1 2 3 4	1 chunk	+18 lines, -0 lines	0 comments	Download
A	pages/index.md	View	2 3 4	1 chunk	+5 lines, -0 lines	0 comments	Download
A	settings.ini	View	2 3 4	1 chunk	+6 lines, -0 lines	0 comments	Download
A	static/fonts/Source-Sans-Pro-300/LICENSE.txt	View	2 3 4	1 chunk	+93 lines, -0 lines	0 comments	Download
A	static/fonts/Source-Sans-Pro-300/Source-Sans-Pro-300.eot	View	2 3 4	Binary file		0 comments	Download
A	static/fonts/Source-Sans-Pro-300/Source-Sans-Pro-300.svg	View	2 3 4	1 chunk	+347 lines, -0 lines	0 comments	Download
A	static/fonts/Source-Sans-Pro-300/Source-Sans-Pro-300.ttf	View	2 3 4	Binary file		0 comments	Download
A	static/fonts/Source-Sans-Pro-300/Source-Sans-Pro-300.woff	View	2 3 4	Binary file		0 comments	Download
A	static/fonts/Source-Sans-Pro-300/Source-Sans-Pro-300.woff2	View	2 3 4	Binary file		0 comments	Download
A	static/fonts/Source-Sans-Pro-400/LICENSE.txt	View	2 3 4	1 chunk	+93 lines, -0 lines	0 comments	Download
A	static/fonts/Source-Sans-Pro-400/Source-Sans-Pro-400.eot	View	2 3 4	Binary file		0 comments	Download
A	static/fonts/Source-Sans-Pro-400/Source-Sans-Pro-400.svg	View	2 3 4	1 chunk	+345 lines, -0 lines	0 comments	Download
A	static/fonts/Source-Sans-Pro-400/Source-Sans-Pro-400.ttf	View	2 3 4	Binary file		0 comments	Download
A	static/fonts/Source-Sans-Pro-400/Source-Sans-Pro-400.woff	View	2 3 4	Binary file		0 comments	Download
A	static/fonts/Source-Sans-Pro-400/Source-Sans-Pro-400.woff2	View	2 3 4	Binary file		0 comments	Download
A	static/fonts/Source-Sans-Pro-700/LICENSE.txt	View	2 3 4	1 chunk	+93 lines, -0 lines	0 comments	Download
A	static/fonts/Source-Sans-Pro-700/Source-Sans-Pro-700.eot	View	2 3 4	Binary file		0 comments	Download
A	static/fonts/Source-Sans-Pro-700/Source-Sans-Pro-700.svg	View	2 3 4	1 chunk	+337 lines, -0 lines	0 comments	Download
A	static/fonts/Source-Sans-Pro-700/Source-Sans-Pro-700.ttf	View	2 3 4	Binary file		0 comments	Download
A	static/fonts/Source-Sans-Pro-700/Source-Sans-Pro-700.woff	View	2 3 4	Binary file		0 comments	Download
A	static/fonts/Source-Sans-Pro-700/Source-Sans-Pro-700.woff2	View	2 3 4	Binary file		0 comments	Download
A	static/js/vendor/html5shiv.min.js	View	1	1 chunk	+4 lines, -0 lines	0 comments	Download
A	static/js/vendor/ie8.min.js	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
A	static/js/vendor/respond.min.js	View	1	1 chunk	+4 lines, -0 lines	0 comments	Download
A	static/scss/base/_font.scss	View	1 2 3 4	1 chunk	+63 lines, -0 lines	0 comments	Download
A	static/scss/base/_reset.scss	View	1 2 3 4	1 chunk	+35 lines, -0 lines	0 comments	Download
A	static/scss/base/_variables.scss	View	1 2 3 4	1 chunk	+66 lines, -0 lines	0 comments	Download
A	static/scss/main.scss	View	1 2 3 4	1 chunk	+28 lines, -0 lines	0 comments	Download
A	templates/default.tmpl	View	1 2 3 4	1 chunk	+49 lines, -0 lines	0 comments	Download

Messages

Total messages: 15

Expand All Messages | Collapse All Messages

juliandoucette

Thanks Ire, Here are a few of my first impressions. https://codereview.adblockplus.org/29474565/diff/29474566/.browserslistrc File .browserslistrc (right): https://codereview.adblockplus.org/29474565/diff/29474566/.browserslistrc#newcode1 ...

June 26, 2017, 12:37 p.m. (2017-06-26 12:37:35 UTC) #2

ire

https://codereview.adblockplus.org/29474565/diff/29474566/.browserslistrc File .browserslistrc (right): https://codereview.adblockplus.org/29474565/diff/29474566/.browserslistrc#newcode1 .browserslistrc:1: # This file is part of help.eyeo.com. On 2017/06/26 ...

June 27, 2017, 2:18 p.m. (2017-06-27 14:18:39 UTC) #3

juliandoucette

Done. https://codereview.adblockplus.org/29474565/diff/29474566/.browserslistrc File .browserslistrc (right): https://codereview.adblockplus.org/29474565/diff/29474566/.browserslistrc#newcode1 .browserslistrc:1: # This file is part of help.eyeo.com. On ...

July 3, 2017, 3:37 p.m. (2017-07-03 15:37:56 UTC) #4

ire

Thanks, made some updates. https://codereview.adblockplus.org/29474565/diff/29474566/.browserslistrc File .browserslistrc (right): https://codereview.adblockplus.org/29474565/diff/29474566/.browserslistrc#newcode1 .browserslistrc:1: # This file is part ...

July 4, 2017, 9:33 a.m. (2017-07-04 09:33:18 UTC) #5

Thanks, made some updates.

https://codereview.adblockplus.org/29474565/diff/29474566/.browserslistrc
File .browserslistrc (right):

https://codereview.adblockplus.org/29474565/diff/29474566/.browserslistrc#new...
.browserslistrc:1: # This file is part of help.eyeo.com.
On 2017/07/03 15:37:54, juliandoucette wrote:
> On 2017/06/27 14:18:37, ire wrote:
> > On 2017/06/26 12:37:33, juliandoucette wrote:
> > > Is there anything in this file that should be unique to this project and
not
> > > others too? If not, then I would suggest implementing this in codingtools
> > > (repo).
> > 
> > Yes this should be company-wide. 
> > 
> > However, I couldn't find a way to use an externally-located .browserlistrc
> file
> > with autoprefixer in this case. Also, perhaps this should be in
> website-defaults
> > instead? I think browserlist is more a frontend website thing. 
> 
> Acknowledged.
> 
> Perhaps it's better to put this in package.json then?

Done.

https://codereview.adblockplus.org/29474565/diff/29474566/static/scss/base/_v...
File static/scss/base/_variables.scss (right):

https://codereview.adblockplus.org/29474565/diff/29474566/static/scss/base/_v...
static/scss/base/_variables.scss:22: $pale-white: #ECECEC;
On 2017/07/03 15:37:54, juliandoucette wrote:
> Acknowledged.
> 
> Note: The names in the style guide were not mandates. Feel free to change them
> wherever it makes sense to match our/your code style.

Okay. I am fine with the names as they are so I will leave them if you have no
objections.

https://codereview.adblockplus.org/29474565/diff/29474566/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29474565/diff/29474566/templates/default.t...
templates/default.tmpl:23: <title>{{ title | translate("page-title", "Meta page
title") }}</title>
On 2017/07/03 15:37:54, juliandoucette wrote:
> > Done. I thought most/all meta tags were handled by the other issue.
> 
> They are, actually. I didn't know that you were considering them. You can
safely
> exclude tags from website-defaults meta [standard.tmpl, social.tmpl, etc] if
you
> plan on including them from website-defaults (which you probably should)
> 
>
https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/stand...

Yes that was the plan. If it's okay by you, I'll leave the metadata I currently
have there for now. Then when the metadata from website defaults are ready, I'll
swap for the ones from there.

https://codereview.adblockplus.org/29474565/diff/29475580/package.json
File package.json (right):

https://codereview.adblockplus.org/29474565/diff/29475580/package.json#newcode12
package.json:12: "website-defaults":
"https://hg.adblockplus.org/website-defaults/archive/tip.tar.gz"
On 2017/07/03 15:37:54, juliandoucette wrote:
> Does this work?

Yes it does

https://codereview.adblockplus.org/29474565/diff/29475580/static/js/vendor/ht...
File static/js/vendor/html5shiv.min.js (right):

https://codereview.adblockplus.org/29474565/diff/29475580/static/js/vendor/ht...
static/js/vendor/html5shiv.min.js:2: * @preserve HTML5 Shiv 3.7.3 | @afarkas
@jdalton @jon_neal @rem | MIT/GPL2 Licensed
On 2017/07/03 15:37:55, juliandoucette wrote:
> I think it would be a good idea to find a way to include these from
> website-defaults.

Yes that's a good idea. A couple points:

1. These files aren't actually part of the website-defaults repository right
now, I had to copy them from acceptableads
2. They would have to be in the actual project directory, so can't be included
in the same way the default SCSS files are. So this seems like something that
should be part of a "website-boilerplate" than the "website-defaults" if that
makes sense. The latter seems to be a set of files that are included into your
project, whereas the former is the basis for your project. Or maybe I'm wrongly
separating these two things which are in fact the same?

https://codereview.adblockplus.org/29474565/diff/29475580/static/scss/base/_f...
File static/scss/base/_font.scss (right):

https://codereview.adblockplus.org/29474565/diff/29475580/static/scss/base/_f...
static/scss/base/_font.scss:23: src:
url("../fonts/Source-Sans-Pro-300/Source-Sans-Pro-300.eot");
On 2017/07/03 15:37:55, juliandoucette wrote:
> I'm getting a 404 when I try to use these fonts.

That's strange, it's working for me. I fixed one of the paths below, so maybe
that has something to do with it? Let me know if you are still experiencing the
issue.

https://codereview.adblockplus.org/29474565/diff/29475580/static/scss/base/_f...
static/scss/base/_font.scss:56: src:
url("/fonts/Source-Sans-Pro-700/Source-Sans-Pro-700.eot?#iefix")
format("embedded-opentype"),
Fixed this URL

https://codereview.adblockplus.org/29474565/diff/29475580/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29474565/diff/29475580/templates/default.t...
templates/default.tmpl:20: lang="{{ locale }}"
On 2017/07/03 15:37:55, juliandoucette wrote:
> Note: I'm probably going to include these html-attributes in website-defaults
> e.g.
> 
> <html <?include meta/html-attributes ?>>
> 
> I don't know if I'm taking it a little too far now :D - let me know what you
> think. 
> 
> My thinking is *every website seems to have these attributes... why not
default
> them?*.

Haha no I don't think you're going too far. In think we should default as much
as possible, takes the uncertainty out of things that shouldn't be uncertain.

https://codereview.adblockplus.org/29474565/diff/29475580/templates/default.t...
templates/default.tmpl:23: <meta charset="utf-8">
On 2017/07/03 15:37:55, juliandoucette wrote:
> Like I mentioned in the last patchset, these are in website-defaults
> meta/standard.tmpl. Sorry about the confusion.

No problem. See my comment about swapping these out when website-defaults meta
are ready.

https://codereview.adblockplus.org/29474565/diff/29475580/templates/default.t...
templates/default.tmpl:29: <!--[if IE 8]>
On 2017/07/03 15:37:56, juliandoucette wrote:
> Like I mentioned in the vendor css, I think should find a way to include these
> from website-defaults (you could exclude them for now and add them later if we
> don't find a way in time).

Acknowledged.

https://codereview.adblockplus.org/29474565/diff/29475580/templates/default.t...
templates/default.tmpl:35: <!--[if lt IE 9]>
On 2017/07/03 15:37:55, juliandoucette wrote:
> See comment above (ie8.js).

Acknowledged.

https://codereview.adblockplus.org/29474565/diff/29475580/templates/default.t...
templates/default.tmpl:40: {{ head | safe }}
On 2017/07/03 15:37:55, juliandoucette wrote:
> I don't think {{ head | safe }} does anything without a {% block head %}.

Done. That's me not understanding how the cms works :P

https://codereview.adblockplus.org/29474565/diff/29475580/templates/default.t...
templates/default.tmpl:43: {{ body | safe }}
On 2017/07/03 15:37:55, juliandoucette wrote:
> I don't think you can extend the body in a child template without a {% block
> body %}.

Done.

juliandoucette

Thanks Ire :) -- Followup below. https://codereview.adblockplus.org/29474565/diff/29474566/static/scss/base/_variables.scss File static/scss/base/_variables.scss (right): https://codereview.adblockplus.org/29474565/diff/29474566/static/scss/base/_variables.scss#newcode22 static/scss/base/_variables.scss:22: $pale-white: #ECECEC; On ...

July 4, 2017, 10:34 a.m. (2017-07-04 10:34:41 UTC) #6

ire

https://codereview.adblockplus.org/29474565/diff/29474566/static/scss/base/_variables.scss File static/scss/base/_variables.scss (right): https://codereview.adblockplus.org/29474565/diff/29474566/static/scss/base/_variables.scss#newcode22 static/scss/base/_variables.scss:22: $pale-white: #ECECEC; On 2017/07/04 10:34:40, juliandoucette wrote: > On ...

July 4, 2017, 11:19 a.m. (2017-07-04 11:19:04 UTC) #7

juliandoucette

https://codereview.adblockplus.org/29474565/diff/29474566/static/scss/base/_variables.scss File static/scss/base/_variables.scss (right): https://codereview.adblockplus.org/29474565/diff/29474566/static/scss/base/_variables.scss#newcode39 static/scss/base/_variables.scss:39: $font-family: "Source Sans Pro", -apple-system, BlinkMacSystemFont, On 2017/07/04 11:19:03, ...

July 4, 2017, 12:24 p.m. (2017-07-04 12:24:52 UTC) #8

ire

https://codereview.adblockplus.org/29474565/diff/29474566/static/scss/base/_variables.scss File static/scss/base/_variables.scss (right): https://codereview.adblockplus.org/29474565/diff/29474566/static/scss/base/_variables.scss#newcode39 static/scss/base/_variables.scss:39: $font-family: "Source Sans Pro", -apple-system, BlinkMacSystemFont, On 2017/07/04 12:24:51, ...

July 4, 2017, 1:36 p.m. (2017-07-04 13:36:43 UTC) #9

juliandoucette

https://codereview.adblockplus.org/29474565/diff/29474566/static/scss/base/_variables.scss File static/scss/base/_variables.scss (right): https://codereview.adblockplus.org/29474565/diff/29474566/static/scss/base/_variables.scss#newcode39 static/scss/base/_variables.scss:39: $font-family: "Source Sans Pro", -apple-system, BlinkMacSystemFont, On 2017/07/04 13:36:43, ...

July 4, 2017, 2:51 p.m. (2017-07-04 14:51:16 UTC) #10

ire

July 5, 2017, 10:14 a.m. (2017-07-05 10:14:11 UTC) #11

juliandoucette

Are you still planning to remove meta data and polyfills from this patchset? Also, it ...

July 5, 2017, 2:26 p.m. (2017-07-05 14:26:30 UTC) #12

ire

On 2017/07/05 14:26:30, juliandoucette wrote: > Are you still planning to remove meta data and ...

July 5, 2017, 9:32 p.m. (2017-07-05 21:32:50 UTC) #13

ire

On 2017/07/05 21:32:50, ire wrote: > Okay I'll push an update. Done.

July 6, 2017, 7:36 a.m. (2017-07-06 07:36:27 UTC) #14

juliandoucette

July 6, 2017, 1:13 p.m. (2017-07-06 13:13:35 UTC) #15

LGTM

https://codereview.adblockplus.org/29474565/diff/29474566/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29474565/diff/29474566/templates/default.t...
templates/default.tmpl:23: <title>{{ title | translate("page-title", "Meta page
title") }}</title>
On 2017/07/04 09:33:15, ire wrote:
> On 2017/07/03 15:37:54, juliandoucette wrote:
> > > Done. I thought most/all meta tags were handled by the other issue.
> > 
> > They are, actually. I didn't know that you were considering them. You can
> safely
> > exclude tags from website-defaults meta [standard.tmpl, social.tmpl, etc] if
> you
> > plan on including them from website-defaults (which you probably should)
> > 
> >
>
https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/stand...
> 
> Yes that was the plan. If it's okay by you, I'll leave the metadata I
currently
> have there for now. Then when the metadata from website defaults are ready,
I'll
> swap for the ones from there. 

Acknowledged.

Expand All Messages | Collapse All Messages