Issue 5413 - Create basic style guide for website-defaults

Issue 5413 - Create basic style guide for website-defaults

[ire, 2017-07-18 06:57:20]

First patch ready :)

https://codereview.adblockplus.org/29491561/diff/29491562/includes/styleguide...
File includes/styleguide/colors/backgrounds.md (right):

https://codereview.adblockplus.org/29491561/diff/29491562/includes/styleguide...
includes/styleguide/colors/backgrounds.md:11: {: .bg-error }
What do you think about adding "success", and "warning" colours as well?

https://codereview.adblockplus.org/29491561/diff/29491562/includes/styleguide...
File includes/styleguide/media/images.md (right):

https://codereview.adblockplus.org/29491561/diff/29491562/includes/styleguide...
includes/styleguide/media/images.md:4: ![Alt
text](https://acceptableads.com/img/placeholders/placeholder-masthead-wide.png){:
.block }
I'm not sure whether these image styles should be included in the style guide or
not. What do you think?

https://codereview.adblockplus.org/29491561/diff/29491562/pages/styleguide.html
File pages/styleguide.html (right):

https://codereview.adblockplus.org/29491561/diff/29491562/pages/styleguide.ht...
pages/styleguide.html:11: <div class="sg-input-group">
Not sure whether this should actually be included in the style guide, but I
found it very helpful while testing. And since we support both directions for
all our sites, it could be useful.

[juliandoucette, 2017-07-24 20:07:55]

This is not quite what I had in mind.

The Acceptable Ads website styleguide includes more than we want to support
across all websites ( more than we want to support on acceptableads.com; in fact
).

I think we should:

1. Change website-defaults into a cms-compatible website
  - See https://gitlab.com/eyeo/website-boilerplate for the minimum required
  - Remove the gulp server task
  - Change the demos into pages
2. Create a styleguide page
  - That only supports what website-defaults supports (so far)

Then, for each thing that we want to add to the styleguide for the help center
we have to ask "should this be in the styleguide for all websites or *just* the
help center?" if all websites -> it should go in website-defaults, if *just* the
help center -> it should go into the help center.

Since we will implement code sharing like this:

<? include styleguide-extra ?>

*searches `./includes`*
*searches includes provided by `[additional-paths]` setting*

We can write the styleguide page in such a way that it can be easily extended by
other websites.

---

Also, as a rule of thumb, do the absolute minimum you can to solve the problem
correctly. e.g. the additional styles you added to the style guide are nice, but
they are not all necessary, and adding them forces me to review and agree or
disagree with them (which takes more time and energy).

[ire, 2017-07-25 08:35:41]

Thanks for the feedback :)

As a broad note, which I'm not sure was clear, my aim with this was to have a
single page where we could visualize how the main/basic components 
of a site could be styled, using acceptableads as an example. It was not
intended that *every* element on this page have a custom style by a site, 
it is more an overview of elements we may expect to be styled. 


On 2017/07/24 20:07:55, juliandoucette wrote:
> This is not quite what I had in mind.
> 
> The Acceptable Ads website styleguide includes more than we want to support
> across all websites ( more than we want to support on
http://acceptableads.com; in fact ).

Got it. Could you be more specific about the sections of the SG you feel should
not be included? 

> I think we should:
> 
> 1. Change website-defaults into a cms-compatible website
>   - See https://gitlab.com/eyeo/website-boilerplate for the minimum required
>   - Remove the gulp server task
>   - Change the demos into pages
> 2. Create a styleguide page
>   - That only supports what website-defaults supports (so far)

A couple points:

1. Just to be clear, are you suggesting that the styleguide should be part of
this new "website-defaults", which is no longer a set of includes but rather a 
boilerplate website?
2. I'm not sure if it was clear that the relevant parts in what i did here are
the styleguide page (pages/styleguide.html) and the styleguide includes. That
is,
this is meant to be included as part of a site, not be a site itself. 


> Then, for each thing that we want to add to the styleguide for the help center
> we have to ask "should this be in the styleguide for all websites or *just*
the
> help center?" if all websites -> it should go in website-defaults, if *just*
the
> help center -> it should go into the help center.
> 
> Since we will implement code sharing like this:
> 
> <? include styleguide-extra ?>
> 
> *searches `./includes`*
> *searches includes provided by `[additional-paths]` setting*
> 
> We can write the styleguide page in such a way that it can be easily extended
by
> other websites.

I agree that this is a good way to go. However, does this mean we intend to
extend website-defaults (and apparently the style guide) to be almost a 
functioning website itself? i.e. will larger components such as a dropdown
component or search field be part of this and be included in the styleguide?
Does this also mean that all our websites will have the same styling for
components such as this?

> Also, as a rule of thumb, do the absolute minimum you can to solve the problem
> correctly. e.g. the additional styles you added to the style guide are nice,
but
> they are not all necessary, and adding them forces me to review and agree or
> disagree with them (which takes more time and energy).

Noted, I may have gotten carried away with designing :p
Besides the more "designy" parts like the colour and  # next to the headings, I
felt I needed a way to differentiate the SG sections from the styled parts 
of the page. If I hadn't added *any* styles, then the SG headings would look
like the rest of the page. 
How would you have preferred I handled this instead?

[juliandoucette, 2017-07-25 13:24:22]

On 2017/07/25 08:35:41, ire wrote:
> As a broad note, which I'm not sure was clear, my aim with this was to have a
> single page where we could visualize how the main/basic components 
> of a site could be styled, using acceptableads as an example. It was not
> intended that *every* element on this page have a custom style by a site, 
> it is more an overview of elements we may expect to be styled. 

Thank you far clarifying. I didn't know this.

> Got it. Could you be more specific about the sections of the SG you feel
should
> not be included? 

This depends on who/what the styleguide is for. 

If we create a styleguide for content developers then we may want to exclude:

- foregrounds
- backgrounds
- forms
- buttons
- floating images
- YouTube videos
- tables

> 1. Just to be clear, are you suggesting that the styleguide should be part of
> this new "website-defaults", which is no longer a set of includes but rather a

> boilerplate website?

Not exactly (Sorry for the confusion).

Sitescripts and I have been discussing how we want to share code across websites
and we have come up with the following:

If we add an `[additional-paths]` section to `settings.ini` and provide a path
to a folder (e.g. an npm dependency) then the cms will look for all of the same
things in the additional-paths that it does the website path (globals, filters,
includes, static, pages, locales, etc). So, if we structure website-defaults
like a website then we can provide all of the resources in the website-defaults
repository to other websites that refer to it as an additional-path. And if we
are going to structure website-defaults like a website for this purpose, then it
logically follows that we should go one step further and add a settings.ini to
website-defaults and use cms to serve demos/etc instead of a gulp server so that
we can test the includes/etc in website-defaults in website-defaults (does that
make sense?)

> I agree that this is a good way to go. However, does this mean we intend to
> extend website-defaults (and apparently the style guide) to be almost a 
> functioning website itself? i.e. will larger components such as a dropdown
> component or search field be part of this and be included in the styleguide?
> Does this also mean that all our websites will have the same styling for
> components such as this?

Yes. Except we haven't decided where to draw the line regarding what
should/shouldn't be included in a styleguide. I think we should start as small
as possible (e.g. just default-content) and then justify adding to it as we go.
Like I said before, this depends on who/what the styleguide is for.

> Noted, I may have gotten carried away with designing :p

No worries. I liked the style 8)

> Besides the more "designy" parts like the colour and  # next to the headings,
I
> felt I needed a way to differentiate the SG sections from the styled parts 
> of the page. If I hadn't added *any* styles, then the SG headings would look
> like the rest of the page. 
> How would you have preferred I handled this instead?

Acknowledged. I would have preferred if you started without any visual
separations and then added them for these reasons in a separate review after we
agreed on what should be in the styleguide.

[juliandoucette, 2017-07-25 13:33:33]

> I would have preferred if you started without any visual
> separations and then added them for these reasons in a separate review after
we
> agreed on what should be in the styleguide.

This also applies (for example) to the rtl switch. I think adding an rtl switch
is a great idea. And we may not even be "solving the problem"  (creating a
styleguide page) "correctly" without one. But if we were to exclude it from this
review and include it in another then I think we would be able to make progress
faster.

[juliandoucette, 2017-07-25 13:38:24]

Also, it seems like I've created two tasks out of this review.

1. Change website-defaults into a CMS compatible website
2. Create a bare bones style guide page including only website-default
components

We could separate these two tasks into two issues/reviews and (potentially)
complete them faster.

(And then you could create separate issues/reviews for what you think should be
added.)

[ire, 2017-07-26 10:34:31]

I think we need to decide *who* this styleguide is for first. I did not actually
consider that this was supposed to
be for content developers. Also, basing it off website-defaults doesn't actually
do what I had originally thought
the purpose of the styleguide was, which is to visualize the styling of the
basic components of any given site.
But I think this is a discussion we can have after implementation of the bare
bones style guide page. 

> Also, it seems like I've created two tasks out of this review.
> 
> 1. Change website-defaults into a CMS compatible website
> 2. Create a bare bones style guide page including only website-default
> components
> 
> We could separate these two tasks into two issues/reviews and (potentially)
> complete them faster.
> 
> (And then you could create separate issues/reviews for what you think should
be
> added.)

Agreed. This is a good way to go. I will create a second issue for the first
task, and 
revise the current issue for the second task.

[ire, 2017-08-03 15:19:07]

Here is a more stripped back version of the styleguide, using just website
defaults CSS. 

I included a compiled version of website defaults CSS for the purpose of testing
here since this styleguide will be dropped into the website-defaults CMS site.
The only files relevant to this issue here are:

- pages/styleguide.html
- includes/styleguide/*

The rest of the files were just necessary for the setup, so you can ignore them
for the purpose of this review.

https://codereview.adblockplus.org/29491561/diff/29504573/pages/styleguide.html
File pages/styleguide.html (right):

https://codereview.adblockplus.org/29491561/diff/29504573/pages/styleguide.ht...
pages/styleguide.html:3: <link rel="stylesheet"
href="/css/website-defaults.css">
This link wouldn't be here in the actual styleguide. Since the actual styleguide
will be part of a CMS-compatible website, the styles will be placed in the
<head>

[juliandoucette, 2017-08-07 14:12:22]

This patchset doesn't seem to apply to website-defaults.

[ire, 2017-08-08 15:54:05]

Ready for review

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
File includes/styleguide/style.html (right):

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/style.html:1: <style>
I added these basic styles for readability. I didn't think it made sense to
style the actual styleguide sections with styles for the website the styleguide
is for, if that makes sense. Let me know if you think otherwise.

[ire, 2017-08-09 08:40:52]

It occurred to me that perhaps we should split the sections into pages, like you
have done with the components. This way, there wouldn't be a dedicated
"styleguide" page, but the homepage will link to the different sections. What do
you think?

[juliandoucette, 2017-08-09 12:06:55]

> It occurred to me that perhaps we should split the sections into pages, like
you have done with the components. This way, there wouldn't be a dedicated
"styleguide" page, but the homepage will link to the different sections. What do
you think?

I think a styleguide is a cheatsheet and a good cheatsheet fits on one page.
Otherwise, why do we even need a styleguide? We have the component demo pages
already.

Acknowledging that this may not be in line with what a "styleguide" is
in-practice in-industry today. Going back to my original point -> It depends
who/what the styleguide is for.

RE: My comments below. I stopped reviewing early. I think you can guess what the
rest of my comments would have been based on the ones I have left so far.

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
File includes/styleguide/style.html (right):

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/style.html:1: <style>
On 2017/08/08 15:54:05, ire wrote:
> I added these basic styles for readability. I didn't think it made sense to
> style the actual styleguide sections with styles for the website the
styleguide
> is for, if that makes sense. Let me know if you think otherwise. 

Acknowledged.

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/style.html:3: .sg-body
I think that the parent `.container` class (which is already applied by
`default.tmpl`) and the body styles in `demo.css` cover what you are doing here.

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/style.html:10: .sg-section
I think we could replace this with a default section spacing scheme in the
future. For now, I think the margin provided by the section headings is enough.

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
File includes/styleguide/typography/code.md (right):

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/typography/code.md:2: ### Code blocks
NIT: We didn't add support for code blocks in website-defaults yet. See my
comment in `inline.md`.

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
File includes/styleguide/typography/headings.md (right):

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/typography/headings.md:4: # Heading 1 {: .h1 }
NIT: I think these {: .h1 } classes are currently misleading. Headings are
styled by level already. And applying a heading class of the same level doesn't
do anything.

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
File includes/styleguide/typography/inline.md (right):

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/typography/inline.md:2: ### Body text
This seems to correlate to the "Body content" section of the default-content
demo page. If there is no good reason for these to be separate then I think we
should use the same include on both pages.

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/typography/inline.md:10: This is <small>small</small> text
I left this out of default-content because it's not supported by Markdown and I
didn't expect content developers to use it semantically correctly.

(I may have been being too harsh. And it may be a good idea to add support for
this in Markdown later via a CMS "converter" or "hook")

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/typography/inline.md:14: This is `inline code`
I left inline-code out of default-content because it wasn't relevant to most of
our websites. I thought it was a better idea to include it in a separate module
with other advanced features like mark, samp, kbd, and syntax highlighting.

https://codereview.adblockplus.org/29491561/diff/29509658/pages/styleguide.html
File pages/styleguide.html (right):

https://codereview.adblockplus.org/29491561/diff/29509658/pages/styleguide.ht...
pages/styleguide.html:5: <div class="sg-body">
Like I said in `style.html` I think the `.container` div above `{{ body }}` in
`default.tmpl` covers this.

https://codereview.adblockplus.org/29491561/diff/29509658/pages/styleguide.ht...
pages/styleguide.html:6: <header class="sg-section">
NIT: A header without additional headings or styles is unnecessary.

https://codereview.adblockplus.org/29491561/diff/29509658/pages/styleguide.ht...
pages/styleguide.html:10: <section class="sg-section">
NIT: I don't think these sections are necessary (they are not wrong... they just
don't add anything of value currently? - the same applies elsewhere.)

[ire, 2017-08-09 16:50:32]

> I think a styleguide is a cheatsheet and a good cheatsheet fits on one page.
> Otherwise, why do we even need a styleguide? We have the component demo pages
> already.

I'm not sure why we need both a styleguide *and* the component demo pages,
considering that the later seems to be a subset of the same information on the
former. 

> RE: My comments below. I stopped reviewing early. I think you can guess what
the
> rest of my comments would have been based on the ones I have left so far.

I think I did. Let me know if I missed something.

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
File includes/styleguide/style.html (right):

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/style.html:3: .sg-body
On 2017/08/09 12:06:53, juliandoucette wrote:
> I think that the parent `.container` class (which is already applied by
> `default.tmpl`) and the body styles in `demo.css` cover what you are doing
here.

Acknowledged. Done.

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/style.html:10: .sg-section
On 2017/08/09 12:06:53, juliandoucette wrote:
> I think we could replace this with a default section spacing scheme in the
> future. For now, I think the margin provided by the section headings is
enough.

Done.

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
File includes/styleguide/typography/code.md (right):

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/typography/code.md:2: ### Code blocks
On 2017/08/09 12:06:53, juliandoucette wrote:
> NIT: We didn't add support for code blocks in website-defaults yet. See my
> comment in `inline.md`.

Done.

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
File includes/styleguide/typography/headings.md (right):

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/typography/headings.md:4: # Heading 1 {: .h1 }
On 2017/08/09 12:06:53, juliandoucette wrote:
> NIT: I think these {: .h1 } classes are currently misleading. Headings are
> styled by level already. And applying a heading class of the same level
doesn't
> do anything.

Done.

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
File includes/styleguide/typography/inline.md (right):

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/typography/inline.md:2: ### Body text
On 2017/08/09 12:06:54, juliandoucette wrote:
> This seems to correlate to the "Body content" section of the default-content
> demo page. If there is no good reason for these to be separate then I think we
> should use the same include on both pages.

What is the purpose of having them on both pages in the first place? Why not
only have the styleguide?

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/typography/inline.md:10: This is <small>small</small> text
On 2017/08/09 12:06:54, juliandoucette wrote:
> I left this out of default-content because it's not supported by Markdown and
I
> didn't expect content developers to use it semantically correctly.
> 
> (I may have been being too harsh. And it may be a good idea to add support for
> this in Markdown later via a CMS "converter" or "hook")

Acknowledged.

https://codereview.adblockplus.org/29491561/diff/29509658/includes/styleguide...
includes/styleguide/typography/inline.md:14: This is `inline code`
On 2017/08/09 12:06:54, juliandoucette wrote:
> I left inline-code out of default-content because it wasn't relevant to most
of
> our websites. I thought it was a better idea to include it in a separate
module
> with other advanced features like mark, samp, kbd, and syntax highlighting.

Acknowledged.

https://codereview.adblockplus.org/29491561/diff/29509658/pages/styleguide.html
File pages/styleguide.html (right):

https://codereview.adblockplus.org/29491561/diff/29509658/pages/styleguide.ht...
pages/styleguide.html:5: <div class="sg-body">
On 2017/08/09 12:06:54, juliandoucette wrote:
> Like I said in `style.html` I think the `.container` div above `{{ body }}` in
> `default.tmpl` covers this.

Done.

https://codereview.adblockplus.org/29491561/diff/29509658/pages/styleguide.ht...
pages/styleguide.html:6: <header class="sg-section">
On 2017/08/09 12:06:54, juliandoucette wrote:
> NIT: A header without additional headings or styles is unnecessary.

Done.

https://codereview.adblockplus.org/29491561/diff/29509658/pages/styleguide.ht...
pages/styleguide.html:10: <section class="sg-section">
On 2017/08/09 12:06:54, juliandoucette wrote:
> NIT: I don't think these sections are necessary (they are not wrong... they
just
> don't add anything of value currently? - the same applies elsewhere.)

Done.

[juliandoucette, 2017-08-09 19:56:18]

> I'm not sure why we need both a styleguide *and* the component demo pages,
considering that the later seems to be a subset of the same information on the
former. 

Ack. I created a demo page per component to document and test each component.
While there is currently complete overlap, I do not think that all future
component's demos/tests/documentation will be appropriate for a styleguide page.

I think we should consider the Styleguide page a component too and use an
include whenever the content within is the same as the content on another
component's page.

Does that make sense? / Do you think that's a good idea too?

[ire, 2017-08-10 09:13:37]

On 2017/08/09 19:56:18, juliandoucette wrote:
> > I'm not sure why we need both a styleguide *and* the component demo pages,
> considering that the later seems to be a subset of the same information on the
> former. 
> 
> Ack. I created a demo page per component to document and test each component.
> While there is currently complete overlap, I do not think that all future
> component's demos/tests/documentation will be appropriate for a styleguide
page.
> 
> I think we should consider the Styleguide page a component too and use an
> include whenever the content within is the same as the content on another
> component's page.
> 
> Does that make sense? / Do you think that's a good idea too?

Yes this makes sense. As a further suggestion, we could even have separate
styleguides for content developers and for us, which will help with the whole
"who is the styleguide for" question.

I will upload a new patchset soon.

[ire, 2017-08-10 09:53:30]

> I will upload a new patchset soon.

Done.

[juliandoucette, 2017-08-10 16:49:32]

Thanks Ire

> Yes this makes sense. As a further suggestion, we could even have separate
styleguides for content developers and for us, which will help with the whole
"who is the styleguide for" question.

Please see my comment on this issue below.

https://codereview.adblockplus.org/29491561/diff/29511565/includes/styleguide...
File includes/styleguide/typography/index.md (right):

https://codereview.adblockplus.org/29491561/diff/29511565/includes/styleguide...
includes/styleguide/typography/index.md:2: ## Typography { #typography }
Regarding the use of Markdown vs HTML:

- We (content and website developers) have agreed to prefer HTML
  - Therefore a styleguide for our sake should be based in HTML
- We (Websites Infrastructure) support Markdown for the sake of
non-(content|website)-developers
  - Mixing markdown with includes may be confusing for non-developers

Therefore I would propose that we create two styleguides:

1. For website developers (with includes and additional components)
2. For non-content-developers (minimal, without includes, without components
e.g. grid)

- Sorry for the confusion :(
- If you agree then we could review one or two of these style guides in this
review

https://codereview.adblockplus.org/29491561/diff/29511565/includes/styleguide...
includes/styleguide/typography/index.md:4: ### Headings { #headings }
NIT: Please separate markdown headings by one blank line

https://codereview.adblockplus.org/29491561/diff/29511565/pages/content.html
File pages/content.html (right):

https://codereview.adblockplus.org/29491561/diff/29511565/pages/content.html#...
pages/content.html:16: <h2 id="body-content">Body content</h2>
NIT: These headings seem to be relevant to the styleguide page too? Why not
include them?

(I can imagine an argument for excluding them on the grounds of increased
flexibility. I don't have a strong opinion.)

https://codereview.adblockplus.org/29491561/diff/29511565/pages/grid.html
File pages/grid.html (right):

https://codereview.adblockplus.org/29491561/diff/29511565/pages/grid.html#new...
pages/grid.html:16: <? include styleguide/grid/all ?>
NIT: "all" is not very descriptive
Suggest: "column-widths-example" (or similar)

https://codereview.adblockplus.org/29491561/diff/29511565/pages/grid.html#new...
pages/grid.html:20: <? include styleguide/grid/all ?>
NIT: Please indent

https://codereview.adblockplus.org/29491561/diff/29511565/pages/grid.html#new...
pages/grid.html:29: <? include styleguide/grid/reversed ?>
NIT: "reversed" what?
Suggest: Replace this include with a "half-column-example" (or similar) include
and move the .reverse class up one level e.g.

<div class="reverse">
  <div class="row"> ...

(Like the example below)

https://codereview.adblockplus.org/29491561/diff/29511565/pages/styleguide.html
File pages/styleguide.html (right):

https://codereview.adblockplus.org/29491561/diff/29511565/pages/styleguide.ht...
pages/styleguide.html:5: <ol>
Note: We aught to create a TOC component :D (later, separate issue)

[ire, 2017-08-11 11:09:49]

Thanks. I've replied to the comment about different styleguides below. In this
patch, I addressed all the other comments about naming, formatting, etc. In the
next patch, I'll separate the 2 styleguides.

https://codereview.adblockplus.org/29491561/diff/29511565/includes/styleguide...
File includes/styleguide/typography/index.md (right):

https://codereview.adblockplus.org/29491561/diff/29511565/includes/styleguide...
includes/styleguide/typography/index.md:2: ## Typography { #typography }
On 2017/08/10 16:49:29, juliandoucette wrote:
> Regarding the use of Markdown vs HTML:
> 
> - We (content and website developers) have agreed to prefer HTML
>   - Therefore a styleguide for our sake should be based in HTML
> - We (Websites Infrastructure) support Markdown for the sake of
> non-(content|website)-developers
>   - Mixing markdown with includes may be confusing for non-developers
> 
> Therefore I would propose that we create two styleguides:
> 
> 1. For website developers (with includes and additional components)
> 2. For non-content-developers (minimal, without includes, without components
> e.g. grid)

Okay this makes sense. If we are intending that non-content-developers will look
at the markup/markdown in order to see how things work, you're right that it
should be without includes and plainly in markdown.

> - Sorry for the confusion :(

No worries :)

> - If you agree then we could review one or two of these style guides in this
> review

I agree :)

https://codereview.adblockplus.org/29491561/diff/29511565/includes/styleguide...
includes/styleguide/typography/index.md:4: ### Headings { #headings }
On 2017/08/10 16:49:29, juliandoucette wrote:
> NIT: Please separate markdown headings by one blank line

Done.

https://codereview.adblockplus.org/29491561/diff/29511565/pages/content.html
File pages/content.html (right):

https://codereview.adblockplus.org/29491561/diff/29511565/pages/content.html#...
pages/content.html:16: <h2 id="body-content">Body content</h2>
On 2017/08/10 16:49:30, juliandoucette wrote:
> NIT: These headings seem to be relevant to the styleguide page too? Why not
> include them?
> 
> (I can imagine an argument for excluding them on the grounds of increased
> flexibility. I don't have a strong opinion.)

They are of different levels. e..g in this page, the "Body content" heading is
an h2, but in the styleguide it's an h3.

https://codereview.adblockplus.org/29491561/diff/29511565/pages/grid.html
File pages/grid.html (right):

https://codereview.adblockplus.org/29491561/diff/29511565/pages/grid.html#new...
pages/grid.html:16: <? include styleguide/grid/all ?>
On 2017/08/10 16:49:30, juliandoucette wrote:
> NIT: "all" is not very descriptive
> Suggest: "column-widths-example" (or similar)

Done. Changed to "all-column-widths", since the other file names describe how
many columns and this is an example demoing all the column widths possible.

https://codereview.adblockplus.org/29491561/diff/29511565/pages/grid.html#new...
pages/grid.html:20: <? include styleguide/grid/all ?>
On 2017/08/10 16:49:31, juliandoucette wrote:
> NIT: Please indent

Done.

https://codereview.adblockplus.org/29491561/diff/29511565/pages/grid.html#new...
pages/grid.html:29: <? include styleguide/grid/reversed ?>
On 2017/08/10 16:49:30, juliandoucette wrote:
> NIT: "reversed" what?
> Suggest: Replace this include with a "half-column-example" (or similar)
include
> and move the .reverse class up one level e.g.
> 
> <div class="reverse">
>   <div class="row"> ...
> 
> (Like the example below)

Done.

https://codereview.adblockplus.org/29491561/diff/29511565/pages/styleguide.html
File pages/styleguide.html (right):

https://codereview.adblockplus.org/29491561/diff/29511565/pages/styleguide.ht...
pages/styleguide.html:5: <ol>
On 2017/08/10 16:49:31, juliandoucette wrote:
> Note: We aught to create a TOC component :D (later, separate issue)

Yup, definitely!

[juliandoucette, 2017-08-16 14:42:02]

> Thanks. I've replied to the comment about different styleguides below. In this
> patch, I addressed all the other comments about naming, formatting, etc. In
the
> next patch, I'll separate the 2 styleguides.

Should I wait for the next Patchset before continuing?

[ire, 2017-08-16 14:46:28]

On 2017/08/16 14:42:02, juliandoucette wrote:
> > Thanks. I've replied to the comment about different styleguides below. In
this
> > patch, I addressed all the other comments about naming, formatting, etc. In
> the
> > next patch, I'll separate the 2 styleguides.
> 
> Should I wait for the next Patchset before continuing?

You should continue first. I'd like to have those other comments resolved so the
next patchset will just be about separating the styleguides

[juliandoucette, 2017-08-20 16:48:39]

Looks good.

https://codereview.adblockplus.org/29491561/diff/29511565/pages/content.html
File pages/content.html (right):

https://codereview.adblockplus.org/29491561/diff/29511565/pages/content.html#...
pages/content.html:16: <h2 id="body-content">Body content</h2>
On 2017/08/11 11:09:47, ire wrote:
> On 2017/08/10 16:49:30, juliandoucette wrote:
> > NIT: These headings seem to be relevant to the styleguide page too? Why not
> > include them?
> > 
> > (I can imagine an argument for excluding them on the grounds of increased
> > flexibility. I don't have a strong opinion.)
> 
> They are of different levels. e..g in this page, the "Body content" heading is
> an h2, but in the styleguide it's an h3. 

Acknowledged.

https://codereview.adblockplus.org/29491561/diff/29511565/pages/grid.html
File pages/grid.html (right):

https://codereview.adblockplus.org/29491561/diff/29511565/pages/grid.html#new...
pages/grid.html:16: <? include styleguide/grid/all ?>
On 2017/08/11 11:09:47, ire wrote:
> On 2017/08/10 16:49:30, juliandoucette wrote:
> > NIT: "all" is not very descriptive
> > Suggest: "column-widths-example" (or similar)
> 
> Done. Changed to "all-column-widths", since the other file names describe how
> many columns and this is an example demoing all the column widths possible.

Acknowledged.

[ire, 2017-08-21 10:19:52]

Thanks. I've separated the the styleguide into a "Content Styleguide" and
"Developer Styleguide".

[juliandoucette, 2017-08-22 14:37:54]

LGTM