Issue 4961 - Fix card group field alignment on acceptableads.com

Issue 4961 - Fix card group field alignment on acceptableads.com

[ire, 2017-06-01 13:14:35]

A couple of points:

Since my fallback solution was not too dissimilar from the current
implementation, I decided to focus on the enhancement and not alter the current
implementation too much.

From what I can tell, I made the assumption that the the .group class always
refers to a group of cards, not a group of any other type of content. Is this
correct? 

In my demo, the markup for each card has three distinct sections (header,
content, footer). Here, the markup is much more variable. So I have 2 options.
Either I alter the markup (add wrappers around parts of the content to form the
three sections), or I style each type of card differently. I'm leaning towards
the former. What do you think? Is there a particular format for the markup of
cards that I may not be aware of?

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
File static/scss/components/_groups.scss (right):

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
static/scss/components/_groups.scss:295: .group-4
I haven't dealt with other group sizes yet (e.g. group-5, group-3). I will do
that in a later patch, after this has been approved.

[juliandoucette, 2017-06-09 14:51:13]

> From what I can tell, I made the assumption that the the .group class always
refers to a group of cards, not a group of any other type of content. Is this
correct? 

No. There are also `.group .item`s. See items 1,2,3 on the homepage.

> In my demo, the markup for each card has three distinct sections (header,
content, footer). Here, the markup is much more variable. So I have 2 options.
Either I alter the markup (add wrappers around parts of the content to form the
three sections), or I style each type of card differently. I'm leaning towards
the former. What do you think? 

I agree. 

It seems we usually have 1-3 of the following sections [.card-heading,
.card-body, .card-footer] and we often want to align them all horizontally.
However, based on our research, this cannot be done easily without using a table
(which comes with it's own set of semantic and responsive design problems). As a
result, we are forced to hard code heights for each of these sections based on
the content within (per screen size). However, using your grid enhancement, we
can make the height of one of these sections flexible. This seems to completely
resolves our issue for some cards (ones with sections that can be absolutely
positioned with a fixed height like .card-footer on the committee page) and
enhances our solution on others.

> Is there a particular format for the markup of cards that I may not be aware
of?

See http://v4-alpha.getbootstrap.com/components/card/#card-layout and
https://zurb.com/university/lessons/foundation-s-flexbox-cards-to-win-the-game

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
static/scss/components/_cards.scss:19: // Variables
Why did you put these variables here and not in _variables.scss?

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
static/scss/components/_cards.scss:20: $card--max-width: 350px;
NIT: Please stay consistent with the variable naming format of existing
variables (one dash). If you think that two dashes is more meaningful then
please make a case for that separately (like you did for media query format).

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
static/scss/components/_cards.scss:39: .card article h3
Why only h3?

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
static/scss/components/_cards.scss:43: @media (min-width: 700px)
It seems like you are doing this to align the button on the committee page. And
perhaps you are planning to position that button (card-footer) absolutely to the
bottom of card later?

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
static/scss/components/_cards.scss:158: .card
When does this apply? It seems to be overpowered by `.group .card, .group
.item`.

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
File static/scss/components/_groups.scss (right):

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
static/scss/components/_groups.scss:295: .group-4
On 2017/06/01 13:14:35, ire wrote:
> I haven't dealt with other group sizes yet (e.g. group-5, group-3). I will do
> that in a later patch, after this has been approved. 

Acknowledged.

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
static/scss/components/_groups.scss:303: .group-4 > div
NIT: I'm guessing that you are following the existing code style here. I should
note that I think we should use a class instead of `> div` here. Sorry about
that :(

[juliandoucette, 2017-06-09 14:54:18]

Additional notes: 

- I may be confusing the terms "align vertically" / "align horizontally"... I
hope you understand what I mean anyway.
- It looks like we can further enhance our solution... See
https://codepen.io/rafibomb/pen/MJWQZZ

[juliandoucette, 2017-06-09 15:04:33]

> - It looks like we can further enhance our solution... See
> https://codepen.io/rafibomb/pen/MJWQZZ

More specifically (sorry), they don't absolutely position their card-footer like
I was suggesting.

[ire, 2017-06-12 14:28:54]

> There are also `.group .item`s. See items 1,2,3 on the homepage.

Noted.


> I agree. 
>
> It seems we usually have 1-3 of the following sections [.card-heading,
> .card-body, .card-footer] and we often want to align them all horizontally.
> However, based on our research, this cannot be done easily without using a
table
> (which comes with it's own set of semantic and responsive design problems). As
a
> result, we are forced to hard code heights for each of these sections based on
> the content within (per screen size). However, using your grid enhancement, we
> can make the height of one of these sections flexible. This seems to
completely
> resolves our issue for some cards (ones with sections that can be absolutely
> positioned with a fixed height like .card-footer on the committee page) and
> enhances our solution on others.

Acknowledged.


> See http://v4-alpha.getbootstrap.com/components/card/#card-layout and
> https://zurb.com/university/lessons/foundation-s-flexbox-cards-to-win-the-game

Seen.


> I may be confusing the terms "align vertically" / "align horizontally"... I
> hope you understand what I mean anyway.

I do :)


> - It looks like we can further enhance our solution... See
https://codepen.io/rafibomb/pen/MJWQZZ
> More specifically (sorry), they don't absolutely position their card-footer
like I was suggesting.

I will look into that

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
static/scss/components/_cards.scss:19: // Variables
On 2017/06/09 14:51:12, juliandoucette wrote:
> Why did you put these variables here and not in _variables.scss?

This was actually an error. I put it here while I was working and didn't
relocate it :/

On another note, I sometimes put variables in the file where they will be used
when they are components like this as it helps with their ability to be
encapsulated. If you're sharing between projects it's easier to share one file.
But that's just a side note, I will move these to _variables.scss.

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
static/scss/components/_cards.scss:20: $card--max-width: 350px;
On 2017/06/09 14:51:12, juliandoucette wrote:
> NIT: Please stay consistent with the variable naming format of existing
> variables (one dash). If you think that two dashes is more meaningful then
> please make a case for that separately (like you did for media query format).

Acknowledged.

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
static/scss/components/_cards.scss:39: .card article h3
On 2017/06/09 14:51:12, juliandoucette wrote:
> Why only h3?

I was targeting a specific case, the committee page. I will change this to use a
class instead.

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
static/scss/components/_cards.scss:43: @media (min-width: 700px)
On 2017/06/09 14:51:12, juliandoucette wrote:
> It seems like you are doing this to align the button on the committee page. 

Yes.

> And perhaps you are planning to position that button (card-footer) absolutely
to the
> bottom of card later?

Because there is only the heading and button, I don't think I need to absolutely
position the footer in this case.

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
static/scss/components/_cards.scss:158: .card
On 2017/06/09 14:51:12, juliandoucette wrote:
> When does this apply? It seems to be overpowered by `.group .card, .group
> .item`.

You're right, my error again, sorry. This should be moved to the `_groups.scss`
so I can target the `.group .card` selector.

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
File static/scss/components/_groups.scss (right):

https://codereview.adblockplus.org/29453600/diff/29453601/static/scss/compone...
static/scss/components/_groups.scss:303: .group-4 > div
On 2017/06/09 14:51:12, juliandoucette wrote:
> NIT: I'm guessing that you are following the existing code style here. I
should
> note that I think we should use a class instead of `> div` here. Sorry about
> that :(

Acknowledged.

[ire, 2017-06-13 16:51:23]

> There are also `.group .item`s. See items 1,2,3 on the homepage.

I see that a group of items also have the class, `.items`. It might be worth
also having a class called `.cards` for a group of cards. Otherwise, the styling
I have done will also apply to items (I don't know if this is what we want).


> It looks like we can further enhance our solution... See
> https://codepen.io/rafibomb/pen/MJWQZZ
> More specifically (sorry), they don't absolutely position their card-footer
> like I was suggesting.

This solution uses Flexbox, so we can't use it for our fallback. Also, we don't
need to absolutely position our footer since out fallback uses fixed heights.

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_cards.scss:169: margin-top: 0 !important;
This is to remove the large top margin that's on the cards on the Solutions page
(e.g. includes/solutions/publishers.md)

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
File static/scss/components/_groups.scss (right):

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_groups.scss:317: .group-5 > div:nth-child(4)
Since this is overwriting a previous style, I stuck with the `> div` selector

[juliandoucette, 2017-06-16 19:39:17]

Thanks Ire!

> I see that a group of items also have the class, `.items`. It might be worth
also having a class called `.cards` for a group of cards. Otherwise, the styling
I have done will also apply to items (I don't know if this is what we want).

No objections.

> This solution uses Flexbox, so we can't use it for our fallback. 

Can you quickly remind me of the downsides of this solution (sorry)? I was under
the impression that it solved one of the problems that we were not able to
(fixed height card sections).

> Also, we don't need to absolutely position our footer since out fallback uses
fixed heights.

Acknowledged.

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_cards.scss:34: .card article .card-heading
Why article?

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_cards.scss:38: @media (min-width: 700px)
Why 700?

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_cards.scss:153: .group .card
I think anything .group should go in _group.scss.

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_cards.scss:161: @media all
Why @media all?

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_cards.scss:169: margin-top: 0 !important;
On 2017/06/13 16:51:23, ire wrote:
> This is to remove the large top margin that's on the cards on the Solutions
page
> (e.g. includes/solutions/publishers.md)

Perhaps this should go in includes/solutions/styles then (if we do need it) ?

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
File static/scss/components/_groups.scss (right):

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_groups.scss:309: .group-5
I'm not sure how/why yet - but this patchset seems to break the centring of the
last card in 5 & 3 card groups.

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_groups.scss:317: .group-5 > div:nth-child(4)
On 2017/06/13 16:51:23, ire wrote:
> Since this is overwriting a previous style, I stuck with the `> div` selector

Acknowledged.

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_groups.scss:319: @media all
Why @media all?

[ire, 2017-06-21 07:47:59]

> Can you quickly remind me of the downsides of this solution (sorry)? 
> I was under the impression that it solved one of the problems that we
> were not able to (fixed height card sections).

No problem at all. There isn't a major downside to this solution, it's just that
the Grid solution is a bit better. So, it doesn't make sense to have 2
enhancement solutions, i.e. Grid-based and Flexbox-based. And, since we are
trying to target older browsers, our fallback solution can't rely on Flexbox
either. So using Flexbox is just a bit in-between the two solutions. For what we
are trying to achieve, our fallback solution has to use fixed heights.

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_cards.scss:34: .card article .card-heading
On 2017/06/16 19:39:16, juliandoucette wrote:
> Why article?

I was selecting the cards on the `pages/committee/index.md`. This replaces the
inline styles that were on that page. (Or are inline styles preferable?)

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_cards.scss:38: @media (min-width: 700px)
On 2017/06/16 19:39:16, juliandoucette wrote:
> Why 700?

Sorry, this was supposed to be the $tablet-breakpoint variable :/

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_cards.scss:153: .group .card
On 2017/06/16 19:39:16, juliandoucette wrote:
> I think anything .group should go in _group.scss.

Acknowledged.

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_cards.scss:161: @media all
On 2017/06/16 19:39:17, juliandoucette wrote:
> Why @media all?

This is to override the various media queries that are applied in the non-grid
solution.

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_cards.scss:169: margin-top: 0 !important;
On 2017/06/16 19:39:16, juliandoucette wrote:
> On 2017/06/13 16:51:23, ire wrote:
> > This is to remove the large top margin that's on the cards on the Solutions
> page
> > (e.g. includes/solutions/publishers.md)
> 
> Perhaps this should go in includes/solutions/styles then (if we do need it) ?

Okay I will move it then

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
File static/scss/components/_groups.scss (right):

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_groups.scss:309: .group-5
On 2017/06/16 19:39:17, juliandoucette wrote:
> I'm not sure how/why yet - but this patchset seems to break the centring of
the
> last card in 5 & 3 card groups. 

I've fixed the centering of the last card in 5 card groups, but I don't see any
uses of 3 card groups (unless you count this page -
http://localhost:5000/en/committee - which is actually a `.group-4`). Plus,
there doesn't seem to be a centering of the last card in 3 card groups looking
at the CSS.

https://codereview.adblockplus.org/29453600/diff/29464663/static/scss/compone...
static/scss/components/_groups.scss:319: @media all
On 2017/06/16 19:39:17, juliandoucette wrote:
> Why @media all?

See comment above

https://codereview.adblockplus.org/29453600/diff/29470555/static/scss/_variab...
File static/scss/_variables.scss (right):

https://codereview.adblockplus.org/29453600/diff/29470555/static/scss/_variab...
static/scss/_variables.scss:169: $card-min-width: 350px;
I realised I was using the wrong variable name (max-width instead of min-width),
so I changed it.

https://codereview.adblockplus.org/29453600/diff/29470555/static/scss/compone...
File static/scss/components/_groups.scss (right):

https://codereview.adblockplus.org/29453600/diff/29470555/static/scss/compone...
static/scss/components/_groups.scss:288: .group.cards
I opted for `.group.cards` instead of just `.cards` here for 2 reasons:

1. This appears to be the way it's already done, see `.group.items` above
2. If I didn't add the group selector, it wouldn't make sense to leave the
styles in the _groups.scss file, I would have to move it to _cards.scss . But,
since the .group styles are defined after in the cascade, they won't be
overridden as we want.

https://codereview.adblockplus.org/29453600/diff/29470555/static/scss/compone...
static/scss/components/_groups.scss:355: @media (min-width: 700px) and
(max-width: $desktop-breakpoint)
I had to use 700px here instead of a variables because that's the exact
breakpoint

[juliandoucette, 2017-07-05 16:15:29]

Can you rebase this patchset please.

[ire, 2017-07-05 21:20:24]

On 2017/07/05 16:15:29, juliandoucette wrote:
> Can you rebase this patchset please.

Done.

[juliandoucette, 2017-07-17 10:40:42]

Rebase again please.

( Sorry for the delay :( )

[ire, 2017-07-17 13:42:45]

On 2017/07/17 10:40:42, juliandoucette wrote:
> Rebase again please.
> 
> ( Sorry for the delay :( )

No problem at all :)

Done.

[juliandoucette, 2017-07-18 22:07:36]

Minor issues, will approve when resolved.

https://codereview.adblockplus.org/29453600/diff/29490668/includes/solutions/...
File includes/solutions/style.html (right):

https://codereview.adblockplus.org/29453600/diff/29490668/includes/solutions/...
includes/solutions/style.html:8: 
NIT: Extra whitespace.

https://codereview.adblockplus.org/29453600/diff/29490668/includes/solutions/...
includes/solutions/style.html:45: .card-footer .m-t-md
I don't think we need .m-t-md in this .card-footer at all do we?

https://codereview.adblockplus.org/29453600/diff/29490668/pages/committee/ind...
File pages/committee/index.html (left):

https://codereview.adblockplus.org/29453600/diff/29490668/pages/committee/ind...
pages/committee/index.html:7: .card h3
I suggest we keep the font-size change. I think the h3 size is too big for four
columns.

These headings are currently overflowing their cards in this patchset.

https://codereview.adblockplus.org/29453600/diff/29490668/pages/committee/ind...
File pages/committee/index.html (right):

https://codereview.adblockplus.org/29453600/diff/29490668/pages/committee/ind...
pages/committee/index.html:6: <div class="container p-b-xl" markdown="1">
- markdown="1" doesn't work inside of .html files.
  - always prefer HTML unless a page/include is intended to be changed regularly
by non-developers
- I think we can replace .p-b-xl with .section here

https://codereview.adblockplus.org/29453600/diff/29490668/pages/committee/ind...
pages/committee/index.html:7: # {{aac-heading[Page Heading] Acceptable Ads
Committee}}
- missing space above heading
- missing hr below heading

https://codereview.adblockplus.org/29453600/diff/29490668/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29453600/diff/29490668/static/scss/compone...
static/scss/components/_cards.scss:39: .card article .card-heading
This selector is too specific (I think it only applies to the committee page). I
suggest solving this problem there.

https://codereview.adblockplus.org/29453600/diff/29490668/static/scss/compone...
File static/scss/components/_groups.scss (right):

https://codereview.adblockplus.org/29453600/diff/29490668/static/scss/compone...
static/scss/components/_groups.scss:303: @supports (display: grid)
Something funky seems to be going on at 992px wide.

https://codereview.adblockplus.org/29453600/diff/29490668/static/scss/compone...
static/scss/components/_groups.scss:327: @media all
NIT: `@media all` doesn't seem to make a difference here because .group.cards is
already quite specific. I think the same applies below.

[ire, 2017-07-20 17:43:26]

Thanks Julian

https://codereview.adblockplus.org/29453600/diff/29490668/includes/solutions/...
File includes/solutions/style.html (right):

https://codereview.adblockplus.org/29453600/diff/29490668/includes/solutions/...
includes/solutions/style.html:8: 
On 2017/07/18 22:07:34, juliandoucette wrote:
> NIT: Extra whitespace.

Done.

https://codereview.adblockplus.org/29453600/diff/29490668/includes/solutions/...
includes/solutions/style.html:45: .card-footer .m-t-md
On 2017/07/18 22:07:34, juliandoucette wrote:
> I don't think we need .m-t-md in this .card-footer at all do we?

You're right! Done.

https://codereview.adblockplus.org/29453600/diff/29490668/pages/committee/ind...
File pages/committee/index.html (left):

https://codereview.adblockplus.org/29453600/diff/29490668/pages/committee/ind...
pages/committee/index.html:7: .card h3
On 2017/07/18 22:07:35, juliandoucette wrote:
> I suggest we keep the font-size change. I think the h3 size is too big for
four
> columns.
> 
> These headings are currently overflowing their cards in this patchset.

Should I still add this change considering that the blog cards will no longer be
on the site (Issue #5373)?

This is also the only case in which the card is just a title and button. So if
we were to add this to the _cards.scss it may never be actually used.

https://codereview.adblockplus.org/29453600/diff/29490668/pages/committee/ind...
File pages/committee/index.html (right):

https://codereview.adblockplus.org/29453600/diff/29490668/pages/committee/ind...
pages/committee/index.html:6: <div class="container p-b-xl" markdown="1">
On 2017/07/18 22:07:35, juliandoucette wrote:
> - markdown="1" doesn't work inside of .html files.
>   - always prefer HTML unless a page/include is intended to be changed
regularly
> by non-developers
> - I think we can replace .p-b-xl with .section here

Noted. Done.

https://codereview.adblockplus.org/29453600/diff/29490668/pages/committee/ind...
pages/committee/index.html:7: # {{aac-heading[Page Heading] Acceptable Ads
Committee}}
On 2017/07/18 22:07:35, juliandoucette wrote:
> - missing space above heading
> - missing hr below heading

Done.

https://codereview.adblockplus.org/29453600/diff/29490668/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29453600/diff/29490668/static/scss/compone...
static/scss/components/_cards.scss:39: .card article .card-heading
On 2017/07/18 22:07:35, juliandoucette wrote:
> This selector is too specific (I think it only applies to the committee page).
I
> suggest solving this problem there.

Same issue here. This applies to the latest blog posts on the committee page
which are removed by #5373

https://codereview.adblockplus.org/29453600/diff/29490668/static/scss/compone...
File static/scss/components/_groups.scss (right):

https://codereview.adblockplus.org/29453600/diff/29490668/static/scss/compone...
static/scss/components/_groups.scss:303: @supports (display: grid)
On 2017/07/18 22:07:36, juliandoucette wrote:
> Something funky seems to be going on at 992px wide.

Done.

https://codereview.adblockplus.org/29453600/diff/29490668/static/scss/compone...
static/scss/components/_groups.scss:327: @media all
On 2017/07/18 22:07:36, juliandoucette wrote:
> NIT: `@media all` doesn't seem to make a difference here because .group.cards
is
> already quite specific. I think the same applies below.

Yep you're right :p . Done.

[juliandoucette, 2017-07-24 16:02:40]

https://codereview.adblockplus.org/29453600/diff/29490668/pages/committee/ind...
File pages/committee/index.html (left):

https://codereview.adblockplus.org/29453600/diff/29490668/pages/committee/ind...
pages/committee/index.html:7: .card h3
On 2017/07/20 17:43:24, ire wrote:
> Should I still add this change considering that the blog cards will no longer
be
> on the site (Issue #5373)?

Yes.

> This is also the only case in which the card is just a title and button. So if
> we were to add this to the _cards.scss it may never be actually used. 

Right. Keep the change on this page. Not in _cards.scss. That way it gets
removed with this page if/when we remove this page.

https://codereview.adblockplus.org/29453600/diff/29490668/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29453600/diff/29490668/static/scss/compone...
static/scss/components/_cards.scss:39: .card article .card-heading
On 2017/07/20 17:43:24, ire wrote:
> On 2017/07/18 22:07:35, juliandoucette wrote:
> > This selector is too specific (I think it only applies to the committee
page).
> I
> > suggest solving this problem there.
> 
> Same issue here. This applies to the latest blog posts on the committee page
> which are removed by #5373

Same answer. Put page specific CSS on pages not in SCSS.

[ire, 2017-07-25 13:33:32]

https://codereview.adblockplus.org/29453600/diff/29490668/pages/committee/ind...
File pages/committee/index.html (left):

https://codereview.adblockplus.org/29453600/diff/29490668/pages/committee/ind...
pages/committee/index.html:7: .card h3
On 2017/07/24 16:02:39, juliandoucette wrote:
> On 2017/07/20 17:43:24, ire wrote:
> > Should I still add this change considering that the blog cards will no
longer
> be
> > on the site (Issue #5373)?
> 
> Yes.
> 
> > This is also the only case in which the card is just a title and button. So
if
> > we were to add this to the _cards.scss it may never be actually used. 
> 
> Right. Keep the change on this page. Not in _cards.scss. That way it gets
> removed with this page if/when we remove this page.

Done.

https://codereview.adblockplus.org/29453600/diff/29490668/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29453600/diff/29490668/static/scss/compone...
static/scss/components/_cards.scss:39: .card article .card-heading
On 2017/07/24 16:02:39, juliandoucette wrote:
> On 2017/07/20 17:43:24, ire wrote:
> > On 2017/07/18 22:07:35, juliandoucette wrote:
> > > This selector is too specific (I think it only applies to the committee
> page).
> > I
> > > suggest solving this problem there.
> > 
> > Same issue here. This applies to the latest blog posts on the committee page
> > which are removed by #5373
> 
> Same answer. Put page specific CSS on pages not in SCSS.

Done.

[juliandoucette, 2017-07-25 13:48:51]

＼(＾O＾)／

LGTM

[ire, 2017-07-25 14:16:13]

On 2017/07/25 13:48:51, juliandoucette wrote:
> ＼(＾O＾)／
> 
> LGTM

(ノ^o^)ノ