Issue 4917 - Shrink space around accent section headings and content when section contains cards on…

Issue 4917 - Shrink space around accent section headings and content when section contains cards on acceptableads.com

[juliandoucette, 2017-06-08 21:28:54]

Thanks Ire!

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

https://codereview.adblockplus.org/29457564/diff/29457565/static/scss/compone...
static/scss/components/_groups.scss:27: .bg-accent .group-container
I'm guessing you applied this to `.bg-accent .group-container` instead of
`.group-container` because the ticket said all groups on accent backgrounds. But
I think this may apply more broadly to all `.group-container`s. Can you check?

https://codereview.adblockplus.org/29457564/diff/29457565/static/scss/compone...
static/scss/components/_groups.scss:39: .bg-accent .group-container > %headings,
Note: I'd like to get away from using %headings because it causes some really
long and complicated selectors. Instead, I would prefer a `.group-heading` and
`.group-body`. But I think this works fine for now. If you agree with my
preference then you may change this. If not, leave it.

[ire, 2017-06-12 08:39:22]

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

https://codereview.adblockplus.org/29457564/diff/29457565/static/scss/compone...
static/scss/components/_groups.scss:27: .bg-accent .group-container
On 2017/06/08 21:28:54, juliandoucette wrote:
> I'm guessing you applied this to `.bg-accent .group-container` instead of
> `.group-container` because the ticket said all groups on accent backgrounds.
But
> I think this may apply more broadly to all `.group-container`s. Can you check?

It looks like the `.group-container` has, so-far, always been placed within a
`.bg-accent` in the markup. So, I will just apply the styles to the
`.group-container` selector alone.

https://codereview.adblockplus.org/29457564/diff/29457565/static/scss/compone...
static/scss/components/_groups.scss:39: .bg-accent .group-container > %headings,
On 2017/06/08 21:28:54, juliandoucette wrote:
> Note: I'd like to get away from using %headings because it causes some really
> long and complicated selectors. Instead, I would prefer a `.group-heading` and
> `.group-body`. But I think this works fine for now. If you agree with my
> preference then you may change this. If not, leave it.

I agree with you that this should be changed. But, perhaps in a separate issue
so we can refactor all uses of the %headings selector?

[juliandoucette, 2017-06-16 18:25:30]

LGTM - Please see comment below before pushing.

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

https://codereview.adblockplus.org/29457564/diff/29457565/static/scss/compone...
static/scss/components/_groups.scss:39: .bg-accent .group-container > %headings,
On 2017/06/12 08:39:21, ire wrote:
> On 2017/06/08 21:28:54, juliandoucette wrote:
> > Note: I'd like to get away from using %headings because it causes some
really
> > long and complicated selectors. Instead, I would prefer a `.group-heading`
and
> > `.group-body`. But I think this works fine for now. If you agree with my
> > preference then you may change this. If not, leave it.
> 
> I agree with you that this should be changed. But, perhaps in a separate issue
> so we can refactor all uses of the %headings selector?

No objections. Please create the issue before marking this Done if you would
like to do so.

[ire, 2017-06-19 09:35:01]

On 2017/06/16 18:25:30, juliandoucette wrote:
> No objections. Please create the issue before marking this Done if you would
> like to do so.

Done - https://issues.adblockplus.org/ticket/5327