Issue 29536625: Issue 5103 - Added padding to 1/2 column images on acceptableads.com

Issue 29536625: Issue 5103 - Added padding to 1/2 column images on acceptableads.com (Closed)

Created:
Sept. 5, 2017, 12:53 p.m. by juliandoucette

Modified:
Sept. 12, 2017, 1:27 p.m.

Reviewers:
ire

Base URL:
https://hg.adblockplus.org/web.acceptableads.com

Visibility:
Public.

Description

Issue 5103 - Added padding to 1/2 column images on acceptableads.com

Patch Set 1 #

Total comments: 8

Created: Sept. 5, 2017, 12:53 p.m.

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+63 lines, -23 lines)			Patch
	M	includes/index/style.html	View		1 chunk	+45 lines, -22 lines	2 comments	Download
	M	pages/about/index.html	View		2 chunks	+18 lines, -1 line	6 comments	Download

Messages

Total messages: 5

Expand All Messages | Collapse All Messages

ire

Thanks Julian. Here are my initial comments https://codereview.adblockplus.org/29536625/diff/29536626/includes/index/style.html File includes/index/style.html (right): https://codereview.adblockplus.org/29536625/diff/29536626/includes/index/style.html#newcode3 includes/index/style.html:3: .masthead-img img ...

Sept. 6, 2017, 10:56 p.m. (2017-09-06 22:56:58 UTC) #2

juliandoucette

Thanks Ire! I may have been a little too lazy and/or opinionated about this one. ...

Sept. 11, 2017, 4:15 p.m. (2017-09-11 16:15:59 UTC) #3

ire

> Thanks Ire! > > I may have been a little too lazy and/or opinionated ...

Sept. 12, 2017, 8:14 a.m. (2017-09-12 08:14:18 UTC) #4

> Thanks Ire!
>
> I may have been a little too lazy and/or opinionated about this one.
> Please let me know what you think and I will update my Patchset
> accordingly.

Haha no worries. I've added my comments. 

Also, I noticed that the blog posts also have a 1/2 column image like on About
page. Should that be updated as well?

https://codereview.adblockplus.org/29536625/diff/29536626/pages/about/index.html
File pages/about/index.html (right):

https://codereview.adblockplus.org/29536625/diff/29536626/pages/about/index.h...
pages/about/index.html:9: max-width: 60%;
On 2017/09/11 16:15:59, juliandoucette wrote:
> On 2017/09/06 22:56:58, ire wrote:
> > Why 60%? Should we use padding instead, as done on the homepage? And if so,
> > maybe there is a way to share the styling of these images across both
> 
> This was opinionated. I thought 60% looked right across sizes where a standard
> space length of padding did not. Do you disagree?

I think the size of the image looks good at 60%. My comment was mainly about
finding a way to share the style across both pages. I think a similar size to
60% can be achieved by using 64px padding on left and the right. But, if the
homepage will likely use a different size, there may not be point to trying to
share the styles. I will leave it up to you how you want to handle this.

https://codereview.adblockplus.org/29536625/diff/29536626/pages/about/index.h...
pages/about/index.html:17: display: none;
On 2017/09/11 16:15:59, juliandoucette wrote:
> On 2017/09/06 22:56:58, ire wrote:
> > Why is this image completely removed on smaller screens?
> 
> This was opinionated. I thought that it didn't look good at any size on
smaller
> screens. Do you disagree?

This is also opinionated, but I think it looks good when it's much smaller
(maybe max-width: 200px), aligned left, and placed *above* the title
("Acceptable Ads..."). The placement in particular is what makes it look awkward
imo.

Perhaps we should get the opinion of Product on this?

juliandoucette

Sept. 12, 2017, 1:27 p.m. (2017-09-12 13:27:01 UTC) #5

Ack - I'm going to see if I can come up with a more general solution for this.

Expand All Messages | Collapse All Messages