Issue 4629 - Default website boilerplate

We would like to implement a default website boilerplate that developers can use as a starting point for new website implementations.

COLLABORATOR=i.aderinokun@eyeo.com

[juliandoucette, 2016-12-07 13:39:29]

On 2016/12/07 13:29:02, juliandoucette wrote:

@Manvel here is a first draft of the boilerplate including standardized meta
data

@Lisa I made you a reviewer because this boilerplate is for all developers
(website & content). I'd like you to review it's documentation and approve that
you are comfortable using it.

(The documentation has not been added yet because it's unclear what we need
where. I will ask to consult you about this soon.)

[saroyanm, 2017-01-11 12:04:41]

https://codereview.adblockplus.org/29366993/diff/29366994/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29366993/diff/29366994/gulpfile.js#newcode31
gulpfile.js:31: gulp.watch("static/scss/*.scss", ["scss"]);
Getting current error:
Error: watch
/home/saroyanm/projects/abp/websites/website-defaults/boilerplate/static/scss/
ENOSPC

Can you please confirm that you don't get any error running gulp ?

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:6: website-boilerplate is free software: you can
redistribute it and/or
Should we use our general license header we use in website projects, while this
is going to be a starting point for our websites ?

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:30: <meta property="og:locale:alternative" content="{{
locales }}">
The way I understand the "og:locale:alternative" that it lists all other
languages that the page is available in, but here seems like we are just
duplicating same locale.

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:31: <meta name="twitter:site" content="{{
site.twitter_handle }}">
What about using other twitter cards ? We can use same title/description/image
we specified above.

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:33: <link rel="apple-touch-icon" sizes="180x180"
href="/img/apple-touch-icon.png">
I think it make sense to use placeholders for the missing images.

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:36: <link rel="manifest" href="/json/manifest.json">
I think this only useful for web apps, also i can't see manifest.json included.

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:38: <meta name="theme-color" content="#ffffff">
Detail: let's keep meta tags together, I suggest rearrange them.

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:42: <link rel="stylesheet" href="/css/main.css">
Is there a particular reason why you have put the main.css in the middle of IE
scripts ?

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:63: <!-- sitemap -->
detail: It's a footer content, not sitemap.

[juliandoucette, 2017-03-03 11:17:44]

I added these notes a while ago but I was unable to publish them because
rietveld was down.

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:6: website-boilerplate is free software: you can
redistribute it and/or
On 2017/01/11 12:04:41, saroyanm wrote:
> Should we use our general license header we use in website projects, while
this
> is going to be a starting point for our websites ?

I don't understand the question. If this boilerplate belongs to it's own
repository, it is it's own project, and needs it's own project name inside it's
licence header.

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:30: <meta property="og:locale:alternative" content="{{
locales }}">
On 2017/01/11 12:04:41, saroyanm wrote:
> The way I understand the "og:locale:alternative" that it lists all other
> languages that the page is available in, but here seems like we are just
> duplicating same locale.

IDK if this was recently changed, but it looks like we should create an
og:locale:alternate for each alternative languange. Not one
og:locale:alternative list of languages.

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:31: <meta name="twitter:site" content="{{
site.twitter_handle }}">
On 2017/01/11 12:04:40, saroyanm wrote:
> What about using other twitter cards ? We can use same title/description/image
> we specified above.

I think that the default card type is OK. And we can override it by adding to
the head block in pages.

The twitter:site refers to the twitter handle (username) of the site though.

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:33: <link rel="apple-touch-icon" sizes="180x180"
href="/img/apple-touch-icon.png">
On 2017/01/11 12:04:40, saroyanm wrote:
> I think it make sense to use placeholders for the missing images.

I just excluded images from this review.

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:36: <link rel="manifest" href="/json/manifest.json">
On 2017/01/11 12:04:40, saroyanm wrote:
> I think this only useful for web apps, also i can't see manifest.json
included.

Acknowledged.

The manifest is also used to specify tile images (favicons).

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:38: <meta name="theme-color" content="#ffffff">
On 2017/01/11 12:04:40, saroyanm wrote:
> Detail: let's keep meta tags together, I suggest rearrange them.

Acknowledged.

I have no strong opinion. This is grouped with the others because it relates to
the favicon. The theme-color is used to define the background color of the
favicon.

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:42: <link rel="stylesheet" href="/css/main.css">
On 2017/01/11 12:04:41, saroyanm wrote:
> Is there a particular reason why you have put the main.css in the middle of IE
> scripts ?

Acknowledged.

IDK. I will move it above IE8 condition.

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:63: <!-- sitemap -->
On 2017/01/11 12:04:41, saroyanm wrote:
> detail: It's a footer content, not sitemap.

Acknowledged.

I had originally planned to implement a simple sitemap in the footer. I will
remove this.

[ire, 2017-06-22 16:14:25]

My initial comments

https://codereview.adblockplus.org/29366993/diff/29366994/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29366993/diff/29366994/gulpfile.js#newcode31
gulpfile.js:31: gulp.watch("static/scss/*.scss", ["scss"]);
This should watch `static/scss/**/*.scss` so that .scss files in sub-directories
will also trigger the task 

Also, is there a reason we are not using the compressed output style option to
minify the output?

https://codereview.adblockplus.org/29366993/diff/29366994/gulpfile.js#newcode31
gulpfile.js:31: gulp.watch("static/scss/*.scss", ["scss"]);
On 2017/01/11 12:04:40, saroyanm wrote:
> Can you please confirm that you don't get any error running gulp ? 

I can confirm that I don't get any error running gulp

https://codereview.adblockplus.org/29366993/diff/29366994/static/scss/main.scss
File static/scss/main.scss (right):

https://codereview.adblockplus.org/29366993/diff/29366994/static/scss/main.sc...
static/scss/main.scss:17: */
The generated CSS ends up with several of these licence headers. Perhaps we
should find a way to strip them out and apply only one to the final main.css
file?

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:53: <header id="header">
I don't think this (or the main or footer) should have an ID as a default. And
particularly a generic id like "header"

[juliandoucette, 2017-06-22 18:23:52]

Sorry Ire - I should have noted that this is not ready for review. There is
another version of this project here https://gitlab.com/eyeo/website-boilerplate
that we will build upon in the future. I'm going to close this review now and
open new issues to add on the new boilerplate linked in the future.

https://codereview.adblockplus.org/29366993/diff/29366994/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29366993/diff/29366994/gulpfile.js#newcode31
gulpfile.js:31: gulp.watch("static/scss/*.scss", ["scss"]);
On 2017/06/22 16:14:24, ire wrote:
> This should watch `static/scss/**/*.scss` so that .scss files in
sub-directories
> will also trigger the task 
> 
> Also, is there a reason we are not using the compressed output style option to
> minify the output?

Acknowledged.

Also, no good reason. Note: If we minify then we need to [include license
headers, a license table, or license magnet links, and source maps] to stay free
and open source and librejs compatible.

https://codereview.adblockplus.org/29366993/diff/29366994/static/scss/main.scss
File static/scss/main.scss (right):

https://codereview.adblockplus.org/29366993/diff/29366994/static/scss/main.sc...
static/scss/main.scss:17: */
On 2017/06/22 16:14:24, ire wrote:
> The generated CSS ends up with several of these licence headers. Perhaps we
> should find a way to strip them out and apply only one to the final main.css
> file?

Acknowledged.

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29366993/diff/29366994/templates/default.t...
templates/default.tmpl:53: <header id="header">
On 2017/06/22 16:14:24, ire wrote:
> I don't think this (or the main or footer) should have an ID as a default. And
> particularly a generic id like "header" 

Acknowledged.

[ire, 2017-06-23 07:23:32]

On 2017/06/22 18:23:52, juliandoucette wrote:
> Sorry Ire - I should have noted that this is not ready for review. There is
> another version of this project here
https://gitlab.com/eyeo/website-boilerplate
> that we will build upon in the future. I'm going to close this review now and
> open new issues to add on the new boilerplate linked in the future.

Okay, I'll have a look at the gitlab repo