Issue 5534 - Create Contact Section for help.eyeo.com

Issue 5534 - Create Contact Section for help.eyeo.com

[ire, 2017-08-18 12:34:30]

Ready for review. This is against the Product Help Template review (which itself
is against the Header Component review)

https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.html
File includes/contact.html (right):

https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.ht...
includes/contact.html:19: <img class="heading-icon"
src="/img/png/logo-facebook.png" srcset="/img/svg/logo-facebook.svg 2x" alt="">
I left a null alt attribute here since the text will have essentially the same
content

https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.ht...
includes/contact.html:20: Facebook
Do we translate this?

https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/layout/...
static/scss/layout/_body.scss:61: .section-accent
These styles are pretty much exclusive to the contact section. I'm considering
changing this to a #contact selector instead. Any thoughts?

[juliandoucette, 2017-08-30 11:33:06]

Thanks Ire :)

Here are my first impressions.

https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.html
File includes/contact.html (right):

https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.ht...
includes/contact.html:5: {{contact[heading] Still looking for help?}}
NIT/Suggest: ID contact-heading (I know... DRY but this ID is a little
misleading?)

https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.ht...
includes/contact.html:9: <section class="column one-half section">
Note: I think we should be consistent about whether we add .section
before/after/inside rows and columns

(I'm not saying that the way you did it here is wrong, I'm just thinking out
loud. Feedback is welcome.)

https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.ht...
includes/contact.html:19: <img class="heading-icon"
src="/img/png/logo-facebook.png" srcset="/img/svg/logo-facebook.svg 2x" alt="">
On 2017/08/18 12:34:30, ire wrote:
> I left a null alt attribute here since the text will have essentially the same
> content

NIT/Suggest: Describe the logo. It's a "rounded square facebook hieroglyph".
Blind people appreciate these details (in my experience working on a website for
a group of blind people). Even when they are not functional. This is based on
subjective experience; of course. We may be able to find contradictory evidence
and/or philosophy.

https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.ht...
includes/contact.html:20: Facebook
On 2017/08/18 12:34:30, ire wrote:
> Do we translate this?

Good question. I don't think it matters. The only thing that matters is whether
we are consistent about translating things like it or not. We could translate it
by default and rely on the CMS to <fix> it by whitelist. Or we could not
translate it. *I think it requires less thinking to translate everything.*

https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/base/_u...
File static/scss/base/_utilities.scss (right):

https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/base/_u...
static/scss/base/_utilities.scss:99: .underlined
NIT: "underlined" doesn't say anything about space. I suggest adding border to
underlined and modifying another selector with space e.g. [ul.underlined,
header.underlined]. Otherwise we are setting ourselves up for a mistake e.g.
adding .underlined to span.

https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/layout/...
static/scss/layout/_body.scss:35: .wrapper
NIT: Wouldn't it would be simpler to just add margins to these wrappers and rely
on the default grid behaviour. e.g.

.container
  .less_wide_section (margin 100px)
    .row
       .col (etc)
   .even_less_wide_section (margin 200px)
     .row
      .col (etc)

where .less_wide_section and .even_less_wide_section only have (or have
different) margin inside min-width desktop or tablet.

(names are for demonstration purposes only)

---

Afterthought: I see the beauty in the solution that you implemented. I will
think about this a little longer and await your feedback. The above
NIT/Suggestion is not a strong opinion.

https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/layout/...
static/scss/layout/_body.scss:61: .section-accent
On 2017/08/18 12:34:30, ire wrote:
> These styles are pretty much exclusive to the contact section. I'm considering
> changing this to a #contact selector instead. Any thoughts?

No strong opinion. I think it makes sense to have an .accent-bg class in
website-defaults that can modify .section. And I wouldn't mind extending that by
class or ID in a specific website.

https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/layout/...
static/scss/layout/_body.scss:70: .section-accent a
NIT: I think that these links are link buttons at the bottom of cards and this
selector is too general. We could have text links inside the text content of
these cards.

https://codereview.adblockplus.org/29519587/diff/29519588/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29519587/diff/29519588/templates/default.t...
templates/default.tmpl:50: <? include contact ?>
This section doesn't exist on all pages (e.g. Imprint). I suggest extending a
minimal.tmpl in default.tmpl if you want to put this include in default.tmpl.

[ire, 2017-09-25 08:47:36]

Updates ready for review :)

https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.html
File includes/contact.html (right):

https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.ht...
includes/contact.html:5: {{contact[heading] Still looking for help?}}
On 2017/08/30 11:33:06, juliandoucette wrote:
> NIT/Suggest: ID contact-heading (I know... DRY but this ID is a little
> misleading?)

Done.

https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.ht...
includes/contact.html:9: <section class="column one-half section">
On 2017/08/30 11:33:06, juliandoucette wrote:
> Note: I think we should be consistent about whether we add .section
> before/after/inside rows and columns
> 
> (I'm not saying that the way you did it here is wrong, I'm just thinking out
> loud. Feedback is welcome.)

I think it depends on where we want the spacing. I don't know that we can have a
rule that applies in all cases.

https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.ht...
includes/contact.html:19: <img class="heading-icon"
src="/img/png/logo-facebook.png" srcset="/img/svg/logo-facebook.svg 2x" alt="">
On 2017/08/30 11:33:06, juliandoucette wrote:
> On 2017/08/18 12:34:30, ire wrote:
> > I left a null alt attribute here since the text will have essentially the
same
> > content
> 
> NIT/Suggest: Describe the logo. It's a "rounded square facebook hieroglyph".
> Blind people appreciate these details (in my experience working on a website
for
> a group of blind people). Even when they are not functional. This is based on
> subjective experience; of course. We may be able to find contradictory
evidence
> and/or philosophy. 

Done. I haven't heard any evidence on the contrary, and your suggestion sounds
helpful.

https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.ht...
includes/contact.html:20: Facebook
On 2017/08/30 11:33:06, juliandoucette wrote:
> On 2017/08/18 12:34:30, ire wrote:
> > Do we translate this?
> 
> Good question. I don't think it matters. The only thing that matters is
whether
> we are consistent about translating things like it or not. We could translate
it
> by default and rely on the CMS to <fix> it by whitelist. Or we could not
> translate it. *I think it requires less thinking to translate everything.*

I agree with you that translating everything makes sense. Done.

https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/base/_u...
File static/scss/base/_utilities.scss (right):

https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/base/_u...
static/scss/base/_utilities.scss:99: .underlined
On 2017/08/30 11:33:06, juliandoucette wrote:
> NIT: "underlined" doesn't say anything about space. I suggest adding border to
> underlined and modifying another selector with space e.g. [ul.underlined,
> header.underlined]. Otherwise we are setting ourselves up for a mistake e.g.
> adding .underlined to span.

Hmm, perhaps the issue is with the name `.underlined` then. Because the goal
here is to create the grouping of both the underline and the spacing. Maybe the
class should be called `.section-underlined`?

https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/layout/...
static/scss/layout/_body.scss:35: .wrapper
On 2017/08/30 11:33:06, juliandoucette wrote:
> NIT: Wouldn't it would be simpler to just add margins to these wrappers and
rely
> on the default grid behaviour. e.g.
> 
> .container
>   .less_wide_section (margin 100px)
>     .row
>        .col (etc)
>    .even_less_wide_section (margin 200px)
>      .row
>       .col (etc)
> 
> where .less_wide_section and .even_less_wide_section only have (or have
> different) margin inside min-width desktop or tablet.
> 
> (names are for demonstration purposes only)
> 
> ---
> 
> Afterthought: I see the beauty in the solution that you implemented. I will
> think about this a little longer and await your feedback. The above
> NIT/Suggestion is not a strong opinion.

I modified this slightly to make use of the `container` class.

https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/layout/...
static/scss/layout/_body.scss:61: .section-accent
On 2017/08/30 11:33:06, juliandoucette wrote:
> On 2017/08/18 12:34:30, ire wrote:
> > These styles are pretty much exclusive to the contact section. I'm
considering
> > changing this to a #contact selector instead. Any thoughts?
> 
> No strong opinion. I think it makes sense to have an .accent-bg class in
> website-defaults that can modify .section. And I wouldn't mind extending that
by
> class or ID in a specific website.

I'll leave it the way it is for now then.

https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/layout/...
static/scss/layout/_body.scss:70: .section-accent a
On 2017/08/30 11:33:06, juliandoucette wrote:
> NIT: I think that these links are link buttons at the bottom of cards and this
> selector is too general. We could have text links inside the text content of
> these cards.

I changed this to a horizontal list, and styled the list items instead

https://codereview.adblockplus.org/29519587/diff/29519588/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29519587/diff/29519588/templates/default.t...
templates/default.tmpl:50: <? include contact ?>
On 2017/08/30 11:33:06, juliandoucette wrote:
> This section doesn't exist on all pages (e.g. Imprint). I suggest extending a
> minimal.tmpl in default.tmpl if you want to put this include in default.tmpl.

Done.

https://codereview.adblockplus.org/29519587/diff/29555634/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29519587/diff/29555634/templates/default.t...
templates/default.tmpl:6: <? include contact ?>
I'm having an issue here. The contact section doesn't show up on the product
help home page (i.e. pages that use a template that extends this template). Is
there an issue with the cms not including things from extended templates?

[juliandoucette, 2017-09-25 12:15:22]

I haven't reviewed this yet. Your question caught my eye.

https://codereview.adblockplus.org/29519587/diff/29555634/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29519587/diff/29555634/templates/default.t...
templates/default.tmpl:6: <? include contact ?>
On 2017/09/25 08:47:36, ire wrote:
> I'm having an issue here. The contact section doesn't show up on the product
> help home page (i.e. pages that use a template that extends this template). Is
> there an issue with the cms not including things from extended templates?

The contents of blocks are replaced; not extended. e.g.

# parent.tmpl

{% block test %}
  Parent text
{% blockend %}

# child.tmpl

{% extends "templates/parent" %}

{% block test %}
  Child text
{% endblock %}

# results

Child text

NOT

Parent text
Child text

If you want Parent text AND Child text to appear then Parent text must be
outside the block that is overwritten (or in a separate block).

Does that make sense?

(You are overwriting the contents of the "body" block in the "product-home"
template.)

[ire, 2017-09-25 14:02:25]

https://codereview.adblockplus.org/29519587/diff/29555634/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29519587/diff/29555634/templates/default.t...
templates/default.tmpl:6: <? include contact ?>
On 2017/09/25 12:15:22, juliandoucette wrote:
> On 2017/09/25 08:47:36, ire wrote:
> > I'm having an issue here. The contact section doesn't show up on the product
> > help home page (i.e. pages that use a template that extends this template).
Is
> > there an issue with the cms not including things from extended templates?
> 
> The contents of blocks are replaced; not extended. e.g.
> 
> # parent.tmpl
> 
> {% block test %}
>   Parent text
> {% blockend %}
> 
> # child.tmpl
> 
> {% extends "templates/parent" %}
> 
> {% block test %}
>   Child text
> {% endblock %}
> 
> # results
> 
> Child text
> 
> NOT
> 
> Parent text
> Child text
> 
> If you want Parent text AND Child text to appear then Parent text must be
> outside the block that is overwritten (or in a separate block).
> 
> Does that make sense?
> 
> (You are overwriting the contents of the "body" block in the "product-home"
> template.)

Ah I see. Got it. 

Putting the include outside the "body" block made it not render at all, so I
created a separate block for it.

[juliandoucette, 2017-09-26 15:37:56]

https://codereview.adblockplus.org/29519587/diff/29555748/includes/contact.html
File includes/contact.html (right):

https://codereview.adblockplus.org/29519587/diff/29555748/includes/contact.ht...
includes/contact.html:1: <section class="section-accent">
NIT: I think that this section technically fits the definition of an aside?

https://codereview.adblockplus.org/29519587/diff/29555748/includes/contact.ht...
includes/contact.html:14: <p>
Isn't this text supposed to be muted?

https://codereview.adblockplus.org/29519587/diff/29555748/includes/contact.ht...
includes/contact.html:15: {{contact-social-description[paragraph] Drop us a line
on Twitter or Facebook and get your question answered ASAP.}}
I don't think that this makes sense as header text. It's not a subtitle or
tagline?

https://codereview.adblockplus.org/29519587/diff/29555748/includes/contact.ht...
includes/contact.html:18: <ul class="horizontal-list">
NIT: I think that the design shows less space above this list?

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/base/_u...
File static/scss/base/_utilities.scss (right):

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/base/_u...
static/scss/base/_utilities.scss:113: .underlined
NIT/Suggest:

.underlined
{
   border-bottom: 1px dotted $gray;
}

header.underlined
{
  margin-bottom: ...
  ...
}

"underlined" doesn't say anything about spacing.

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:44: .section-accent
I think that I have been confused about the meaning of "primary", "secondary",
and "accent" colors.

I was thinking:

Primary: The most common scheme e.g. black on white
Secondary: A contrasting scheme to primary e.g. white on dark gray
Accent: A highlighted / colored scheme e.g. white on red (ABP), white on blue
(ABB)

I think that this makes sense. But it's not the same as what is described here
https://material.io/guidelines/style/color.html#color-color-system.

I would call this (.section-accent) a "secondary" section. And it seems Google
would call this a "secondary" or "accent" section. 

Anyway, I'm ok with this class name. But I wanted to point out that I have been
confused. And allow you to give feedback if you think that my working
definitions did or did not make sense. Maybe we should change the
website-default colors to be more true to Google's definitions?

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:48: background-image:
url("/img/png/accent-bg.png");
Can we do this with SVG or triangular borders? (the glow/shadow may not be very
important)

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:59: }
All links are supposed to be blue; aren't they?

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:61: [dir="rtl"]
Note: You could set a negative margin on the list and then apply an equal
positive margin left and right on the list items to avoid rtl styles here; I
think.

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:70: .container-sm
NIT/Suggest: Doesn't ".tablet-width" make more sense?

https://codereview.adblockplus.org/29519587/diff/29555748/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29519587/diff/29555748/templates/default.t...
templates/default.tmpl:8: <? include contact ?>
NIT: I think that it's misleading to place this section within a block called
"footer" because it's not part of the HTML <footer>.

[ire, 2017-09-28 09:43:46]

Thanks Julian! I've made some changes.

https://codereview.adblockplus.org/29519587/diff/29555748/includes/contact.html
File includes/contact.html (right):

https://codereview.adblockplus.org/29519587/diff/29555748/includes/contact.ht...
includes/contact.html:1: <section class="section-accent">
On 2017/09/26 15:37:55, juliandoucette wrote:
> NIT: I think that this section technically fits the definition of an aside?

Yup you're right. Done.

https://codereview.adblockplus.org/29519587/diff/29555748/includes/contact.ht...
includes/contact.html:14: <p>
On 2017/09/26 15:37:55, juliandoucette wrote:
> Isn't this text supposed to be muted?

Yes. But when I add the muted class, the muted text on this background no longer
meets the accessibility requirements. I think I would prefer to leave it black,
and raise an issue with Jeen about the colour separately.

https://codereview.adblockplus.org/29519587/diff/29555748/includes/contact.ht...
includes/contact.html:15: {{contact-social-description[paragraph] Drop us a line
on Twitter or Facebook and get your question answered ASAP.}}
On 2017/09/26 15:37:55, juliandoucette wrote:
> I don't think that this makes sense as header text. It's not a subtitle or
> tagline?

I actually do consider it a subtitle/tagline. I think it reads that way.

https://codereview.adblockplus.org/29519587/diff/29555748/includes/contact.ht...
includes/contact.html:18: <ul class="horizontal-list">
On 2017/09/26 15:37:55, juliandoucette wrote:
> NIT: I think that the design shows less space above this list?

Done.

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/base/_u...
File static/scss/base/_utilities.scss (right):

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/base/_u...
static/scss/base/_utilities.scss:113: .underlined
On 2017/09/26 15:37:55, juliandoucette wrote:
> NIT/Suggest:
> 
> .underlined
> {
>    border-bottom: 1px dotted $gray;
> }
> 
> header.underlined
> {
>   margin-bottom: ...
>   ...
> }
> 
> "underlined" doesn't say anything about spacing.

You made this comment here:
https://codereview.adblockplus.org/29519587/diff/29519588/static/scss/base/_u...
but I don't think you saw my response.

What I will do is leave this class as an underline only, then modify the
.underline class for the #contact section alone. 

I don't think doing `header.underlined` in general is as extensible because we
would have to add the modifier to several tags.

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:44: .section-accent
On 2017/09/26 15:37:56, juliandoucette wrote:
> I think that I have been confused about the meaning of "primary", "secondary",
> and "accent" colors.
> 
> I was thinking:
> 
> Primary: The most common scheme e.g. black on white
> Secondary: A contrasting scheme to primary e.g. white on dark gray
> Accent: A highlighted / colored scheme e.g. white on red (ABP), white on blue
> (ABB)
> 
> I think that this makes sense. But it's not the same as what is described here
> https://material.io/guidelines/style/color.html#color-color-system.
> 
> I would call this (.section-accent) a "secondary" section. And it seems Google
> would call this a "secondary" or "accent" section. 
> 
> Anyway, I'm ok with this class name. But I wanted to point out that I have
been
> confused. And allow you to give feedback if you think that my working
> definitions did or did not make sense. Maybe we should change the
> website-default colors to be more true to Google's definitions?

1. I think I have been confused as well, and I can't seem to decide for myself
which definitions make the most sense.

2. I like your naming scheme, particularly your definition of "accent" (For
Google, "accent" seems to be the same as "secondary"). 

3. By your own naming scheme, this section should be an accent, not a secondary
(the way I understand it anyway. e.g in aa.com, the green sections are "accent")

4. I think the confusion comes from the naming of "secondary". Perhaps we can
change it to be called "primary-contrast" or something like that, and keep
"accent" as "accent" (i.e. remove secondary)

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:48: background-image:
url("/img/png/accent-bg.png");
On 2017/09/26 15:37:55, juliandoucette wrote:
> Can we do this with SVG or triangular borders? (the glow/shadow may not be
very
> important)

Perhaps, but what would be the benefit in this case? (Considering we would lose
the glow/shadow)

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:59: }
On 2017/09/26 15:37:55, juliandoucette wrote:
> All links are supposed to be blue; aren't they?

Done.

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:61: [dir="rtl"]
On 2017/09/26 15:37:56, juliandoucette wrote:
> Note: You could set a negative margin on the list and then apply an equal
> positive margin left and right on the list items to avoid rtl styles here; I
> think.

Nice trick! But the margins between the list items don't collapse, and the
spacing between them gets too big.

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:70: .container-sm
On 2017/09/26 15:37:56, juliandoucette wrote:
> NIT/Suggest: Doesn't ".tablet-width" make more sense?

Yes it makes a lot more sense haha. Done.

(We should probably move this class to website-defaults)

https://codereview.adblockplus.org/29519587/diff/29555748/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29519587/diff/29555748/templates/default.t...
templates/default.tmpl:8: <? include contact ?>
On 2017/09/26 15:37:56, juliandoucette wrote:
> NIT: I think that it's misleading to place this section within a block called
> "footer" because it's not part of the HTML <footer>.

I don't necessarily think "footer" means it must be part of the HTML <footer>. I
see it as the common meaning of footer, which is anything at the bottom of the
page. Plus, "footer" is more vague and will allow for other things besides the
contact section to be in that block.

[juliandoucette, 2017-10-09 22:00:40]

LGTM - I think we can address the issues below separately as you see fit.

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/base/_u...
File static/scss/base/_utilities.scss (right):

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/base/_u...
static/scss/base/_utilities.scss:113: .underlined
On 2017/09/28 09:43:44, ire wrote:
> What I will do is leave this class as an underline only, then modify the
> .underline class for the #contact section alone. 
> 
> I don't think doing `header.underlined` in general is as extensible because we
> would have to add the modifier to several tags.

Agreed.

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:44: .section-accent
On 2017/09/28 09:43:44, ire wrote:
> 3. By your own naming scheme, this section should be an accent, not a
secondary
> (the way I understand it anyway. e.g in http://aa.com, the green sections are
"accent")

I disagree. It would be secondary. And the product colours would be accent.
Secondary didn't necessarily mean "opposite".

> 4. I think the confusion comes from the naming of "secondary". Perhaps we can
> change it to be called "primary-contrast" or something like that, and keep
> "accent" as "accent" (i.e. remove secondary)

Perhaps [primary, secondary, brand]?

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:48: background-image:
url("/img/png/accent-bg.png");
On 2017/09/28 09:43:45, ire wrote:
> Perhaps, but what would be the benefit in this case? (Considering we would
lose
> the glow/shadow)

[smaller file size, less blur]

(This is a NIT)

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:61: [dir="rtl"]
On 2017/09/28 09:43:45, ire wrote:
> Nice trick! But the margins between the list items don't collapse, and the
> spacing between them gets too big.

I was counting on the margins not collapsing.

Assuming $md = 2 * $sm (or similar)
ul (margin: 0 -$sm)
li (margin: 0 $sm)

This will equal out to $md between each.

(Note: A note is less than a NIT :D - You don't have to address this if you
don't want to.)

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:70: .container-sm
On 2017/09/28 09:43:45, ire wrote:
> (We should probably move this class to website-defaults)

Acknowledged.

https://codereview.adblockplus.org/29519587/diff/29555748/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29519587/diff/29555748/templates/default.t...
templates/default.tmpl:8: <? include contact ?>
On 2017/09/28 09:43:46, ire wrote:
> I don't necessarily think "footer" means it must be part of the HTML <footer>.
I
> see it as the common meaning of footer, which is anything at the bottom of the
> page. Plus, "footer" is more vague and will allow for other things besides the
> contact section to be in that block.

I think you're right.

https://codereview.adblockplus.org/29519587/diff/29558588/includes/contact.html
File includes/contact.html (right):

https://codereview.adblockplus.org/29519587/diff/29558588/includes/contact.ht...
includes/contact.html:23: {{facebook[Brand name] Facebook}}
SuperNIT: There is extra whitespace before/after this text that is underlined on
hover. Perhaps we could [drop the underline, or apply the underline to an inner
span].

https://codereview.adblockplus.org/29519587/diff/29558588/includes/contact.ht...
includes/contact.html:29: alt="{{ftwitter-logo[Image alt text] Blue Twitter
hieroglyph}}">
NIT/Suggest: *just* "glyph"

Source: http://wikidiff.com/hieroglyph/glyph

https://codereview.adblockplus.org/29519587/diff/29558588/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29519587/diff/29558588/static/scss/content...
static/scss/content/_typography.scss:35: font-size: $font-size-h2;
Note: We should probably make this configurable via website-defaults

https://codereview.adblockplus.org/29519587/diff/29558588/static/scss/content...
static/scss/content/_typography.scss:47: color: $font-color-link;
Note: This is configurable via website-defaults
https://hg.adblockplus.org/website-defaults/file/tip/scss/_content.scss#l93 -
perhaps it should be *more* configurable e.g. $link-color

[ire, 2017-10-10 18:36:30]

Thanks. I've taken note of the comments that will be addressed in a separate
issue later

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:44: .section-accent
On 2017/10/09 22:00:38, juliandoucette wrote:
> On 2017/09/28 09:43:44, ire wrote:
> > 3. By your own naming scheme, this section should be an accent, not a
> secondary
> > (the way I understand it anyway. e.g in http://aa.com, the green sections
are
> "accent")
> 
> I disagree. It would be secondary. And the product colours would be accent.
> Secondary didn't necessarily mean "opposite".
> 
> > 4. I think the confusion comes from the naming of "secondary". Perhaps we
can
> > change it to be called "primary-contrast" or something like that, and keep
> > "accent" as "accent" (i.e. remove secondary)
> 
> Perhaps [primary, secondary, brand]?

Isn't "brand" "primary"? Haha. I think we should create an issue to discuss this
further, and maybe get input from others as well.

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:48: background-image:
url("/img/png/accent-bg.png");
On 2017/10/09 22:00:38, juliandoucette wrote:
> On 2017/09/28 09:43:45, ire wrote:
> > Perhaps, but what would be the benefit in this case? (Considering we would
> lose
> > the glow/shadow)
> 
> [smaller file size, less blur]
> 
> (This is a NIT)

Fair points. I think I will consider this something that can be looked into post
launch.

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:61: [dir="rtl"]
On 2017/10/09 22:00:38, juliandoucette wrote:
> On 2017/09/28 09:43:45, ire wrote:
> > Nice trick! But the margins between the list items don't collapse, and the
> > spacing between them gets too big.
> 
> I was counting on the margins not collapsing.
> 
> Assuming $md = 2 * $sm (or similar)
> ul (margin: 0 -$sm)
> li (margin: 0 $sm)
> 
> This will equal out to $md between each.
> 
> (Note: A note is less than a NIT :D - You don't have to address this if you
> don't want to.)

Ah I see, I think I was using $md when I tried and the spacing was much too big.

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:61: [dir="rtl"]
On 2017/10/09 22:00:38, juliandoucette wrote:
> On 2017/09/28 09:43:45, ire wrote:
> > Nice trick! But the margins between the list items don't collapse, and the
> > spacing between them gets too big.
> 
> I was counting on the margins not collapsing.
> 
> Assuming $md = 2 * $sm (or similar)
> ul (margin: 0 -$sm)
> li (margin: 0 $sm)
> 
> This will equal out to $md between each.
> 
> (Note: A note is less than a NIT :D - You don't have to address this if you
> don't want to.)

Done.

https://codereview.adblockplus.org/29519587/diff/29558588/includes/contact.html
File includes/contact.html (right):

https://codereview.adblockplus.org/29519587/diff/29558588/includes/contact.ht...
includes/contact.html:29: alt="{{ftwitter-logo[Image alt text] Blue Twitter
hieroglyph}}">
On 2017/10/09 22:00:39, juliandoucette wrote:
> NIT/Suggest: *just* "glyph"
> 
> Source: http://wikidiff.com/hieroglyph/glyph

Done.

https://codereview.adblockplus.org/29519587/diff/29558588/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29519587/diff/29558588/static/scss/content...
static/scss/content/_typography.scss:35: font-size: $font-size-h2;
On 2017/10/09 22:00:39, juliandoucette wrote:
> Note: We should probably make this configurable via website-defaults

I agree

https://codereview.adblockplus.org/29519587/diff/29558588/static/scss/content...
static/scss/content/_typography.scss:47: color: $font-color-link;
On 2017/10/09 22:00:39, juliandoucette wrote:
> Note: This is configurable via website-defaults
> https://hg.adblockplus.org/website-defaults/file/tip/scss/_content.scss#l93 -
> perhaps it should be *more* configurable e.g. $link-color

Yes it probably should, as $accent could apply to other things