Issue 29657629: Issue 6211 - Fixed size of blog post thumbnails on mobile (Closed)
|
Created:
Jan. 5, 2018, 2:46 p.m. by juliandoucette Modified:
Jan. 15, 2018, 5:20 p.m. Reviewers:
ire Base URL:
https://hg.adblockplus.org/web.acceptableads.com Visibility:
Public. |
DescriptionIssue 6211 - Fixed size of blog post thumbnails on mobile
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed #3 #Patch Set 3 : Addressed #3 (forgot to fix 766) #
Total comments: 2
Patch Set 4 : Addressed #5 #
Total comments: 3
MessagesTotal messages: 10
Details: 1. I think the size of this thumbnail on tablet/small-desktop should be 200px not 100px (but we can address this separately - I think 200px fits well for mobile at least) 2. I think that the link text below the thumbnail should be restricted to ~90% or so on tablets - so that it looks less awkward underneath the centered and restricted size of the above thumbnail (but we can address this separately)
Thanks Julian. Comments below. On 2018/01/05 14:50:44, juliandoucette wrote: > 1. I think the size of this thumbnail on tablet/small-desktop should be 200px > not 100px (but we can address this separately - I think 200px fits well for > mobile at least) Ack. > 2. I think that the link text below the thumbnail should be restricted to ~90% > or so on tablets - so that it looks less awkward underneath the centered and > restricted size of the above thumbnail (but we can address this separately) Ack. https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md File pages/blog/index.md (right): https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md... pages/blog/index.md:19: @media (max-width: 766px) Should be 767px https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md... pages/blog/index.md:21: .card a, Why is there a fixed height on the links as well? https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md... pages/blog/index.md:22: .card img { height: 200px; } This isn't relevant now (because most images are almost square), but we should account for images that are landscape. Currently, the width of these images isn't responsive, so if we get an image that is wider than longer, it will overflow https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md... pages/blog/index.md:22: .card img { height: 200px; } I don't think the single-line format should be used if there are two lines of selector? https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md... pages/blog/index.md:27: .card img { max-height: 100px; } NIT: Maybe this should just be height instead of max-height?
New Patchset uploaded. https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md File pages/blog/index.md (right): https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md... pages/blog/index.md:19: @media (max-width: 766px) On 2018/01/08 10:58:44, ire wrote: > Should be 767px Good catch. https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md... pages/blog/index.md:21: .card a, On 2018/01/08 10:58:43, ire wrote: > Why is there a fixed height on the links as well? No idea :/ https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md... pages/blog/index.md:22: .card img { height: 200px; } On 2018/01/08 10:58:43, ire wrote: > This isn't relevant now (because most images are almost square), but we should > account for images that are landscape. Currently, the width of these images > isn't responsive, so if we get an image that is wider than longer, it will > overflow Good catch. https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md... pages/blog/index.md:22: .card img { height: 200px; } On 2018/01/08 10:58:43, ire wrote: > I don't think the single-line format should be used if there are two lines of > selector? Not sure about this :/ (It won't be relevant in the next patchset.) https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md... pages/blog/index.md:27: .card img { max-height: 100px; } On 2018/01/08 10:58:44, ire wrote: > NIT: Maybe this should just be height instead of max-height? I think max-height is better if height/width are auto in-case the image is wide.
https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md File pages/blog/index.md (right): https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md... pages/blog/index.md:22: .card img { height: 200px; } On 2018/01/08 12:45:36, juliandoucette wrote: > On 2018/01/08 10:58:43, ire wrote: > > This isn't relevant now (because most images are almost square), but we should > > account for images that are landscape. Currently, the width of these images > > isn't responsive, so if we get an image that is wider than longer, it will > > overflow > > Good catch. Are you going to address this now? https://codereview.adblockplus.org/29657629/diff/29659583/pages/blog/index.md File pages/blog/index.md (right): https://codereview.adblockplus.org/29657629/diff/29659583/pages/blog/index.md... pages/blog/index.md:21: @media (max-width: 767px) You probably don't need this media query. This could be the default and the media query below overriding
New Patchset uploaded. ":D" in my comments is more a sign of embarrassment than laughter. https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md File pages/blog/index.md (right): https://codereview.adblockplus.org/29657629/diff/29657630/pages/blog/index.md... pages/blog/index.md:22: .card img { height: 200px; } On 2018/01/09 08:26:33, ire wrote: > Are you going to address this now? I thought I did :D Should be addressed now. https://codereview.adblockplus.org/29657629/diff/29659583/pages/blog/index.md File pages/blog/index.md (right): https://codereview.adblockplus.org/29657629/diff/29659583/pages/blog/index.md... pages/blog/index.md:21: @media (max-width: 767px) On 2018/01/09 08:26:34, ire wrote: > You probably don't need this media query. This could be the default and the > media query below overriding Good point :D
On 2018/01/09 14:24:04, juliandoucette wrote: > New Patchset uploaded. > > ":D" in my comments is more a sign of embarrassment than laughter. Haha alright :P https://codereview.adblockplus.org/29657629/diff/29660575/pages/blog/index.md File pages/blog/index.md (right): https://codereview.adblockplus.org/29657629/diff/29660575/pages/blog/index.md... pages/blog/index.md:24: .card h3 { height: 70px; } Why is this here? (This height doesn't actually contain the longer headings if that is what you were going for?)
https://codereview.adblockplus.org/29657629/diff/29660575/pages/blog/index.md File pages/blog/index.md (right): https://codereview.adblockplus.org/29657629/diff/29660575/pages/blog/index.md... pages/blog/index.md:24: .card h3 { height: 70px; } On 2018/01/10 10:08:39, ire wrote: > Why is this here? (This height doesn't actually contain the longer headings if > that is what you were going for?) I knew this would come back to bite me :D This is here because it was there before. IIRC this is leftover from a time past when the height of the header was actually fixed and overflow:hidden. I want(ed) to address this separately. But I kindof dipped my feet in the pond by moving it from .card h3 to @media here (I moved it because it is not helpful on mobile - which I was optimizing for in this issue - IDK if it's *really* helpful on desktop)
LGTM https://codereview.adblockplus.org/29657629/diff/29660575/pages/blog/index.md File pages/blog/index.md (right): https://codereview.adblockplus.org/29657629/diff/29660575/pages/blog/index.md... pages/blog/index.md:24: .card h3 { height: 70px; } On 2018/01/11 12:43:56, juliandoucette wrote: > On 2018/01/10 10:08:39, ire wrote: > > Why is this here? (This height doesn't actually contain the longer headings if > > that is what you were going for?) > > I knew this would come back to bite me :D > > This is here because it was there before. IIRC this is leftover from a time past > when the height of the header was actually fixed and overflow:hidden. I want(ed) > to address this separately. But I kindof dipped my feet in the pond by moving it > from .card h3 to @media here (I moved it because it is not helpful on mobile - > which I was optimizing for in this issue - IDK if it's *really* helpful on > desktop) Haha ack. I don't think it is particularly helpful on desktop, but we don't have to address this now/ever
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
