Issue 29720653: Fixes #30 - Adjustments to hero unit section of index page (Closed)
|
Created:
March 12, 2018, 6:01 p.m. by juliandoucette Modified:
March 29, 2018, 10:15 p.m. Reviewers:
ire Base URL:
https://hg.adblockplus.org/web.adblockplus.org Visibility:
Public. |
DescriptionCOLLABORATOR=i.aderinokun@eyeo.com
Fixes #30 - Adjustments to hero unit section of index page
Patch Set 1 #
Total comments: 15
Patch Set 2 : Addressed #3 #
Total comments: 17
Patch Set 3 : Addressed #6 & #7 #
Total comments: 1
Patch Set 4 : Rebased #
Total comments: 4
Patch Set 5 : Addressed #10 #Patch Set 6 : Addressed #13 & #14 #
MessagesTotal messages: 17
Warning: - This patchset applies on top of: - https://codereview.adblockplus.org/29720642/ - https://codereview.adblockplus.org/29720648/ (Sorry about the inconvinience. I will export a patchset including the above two changesets if you like?) - I will upload screenshots of these changes to the issue on gitlab - Jeen may decide to make additional changes as a result - I consider this a "first draft" of this change - It may be a little messy https://codereview.adblockplus.org/29720653/diff/29720654/includes/hero-downl... File includes/hero-download.html (right): https://codereview.adblockplus.org/29720653/diff/29720654/includes/hero-downl... includes/hero-download.html:27: <img class="video-thumbnail" src="/img/hero-video.jpg" srcset="/img/hero-video-2x.jpg 2x" alt="{{ video-alt-text[alt text] Introduction to Adblock Plus video thumbnail }}"> I forgot to delete video-thumbnail.jpg
On 2018/03/12 18:07:51, juliandoucette wrote: > Warning: > > - This patchset applies on top of: > - https://codereview.adblockplus.org/29720642/ > - https://codereview.adblockplus.org/29720648/ > > (Sorry about the inconvinience. I will export a patchset including the above two > changesets if you like?) No problem, the other reviews will be resolved very soon anyway so no need to go through the effort. https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.cs... static/css/index.css:16: padding: 3em 1em 3em 1em; NIT: Use shorthand `3em 1em` instead https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.cs... static/css/index.css:16: padding: 3em 1em 3em 1em; Shouldn't this just be 3em al around? Spec: "Below 1024px (Below Desktop breakpoint) Ensure exactly 3rem all around section" https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.cs... static/css/index.css:19: #hero-download h1 .avoid-wrap Is there a reason you didn't use `word-wrap`/`word-break` to stop the word breaking? https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.cs... static/css/index.css:52: padding: 6em 1em 5em 1em; NIT: Shorthand `6em 1em 5em` https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.cs... static/css/index.css:52: padding: 6em 1em 5em 1em; Shouldn't this be `6em 1em`? Spec: "Ensure exactly 6rem above and below section" https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.cs... static/css/index.css:101: border: none; The adding/removing of the border causes a shift. Suggest: Instead of removing the border here, add a transparent border of the same width
Detail: I pushed both of these other patchsets to master. I'll merge/rebase and re-upload a new patchset.
Thanks Ire! New patchset up! https://codereview.adblockplus.org/29720653/diff/29720654/includes/hero-downl... File includes/hero-download.html (right): https://codereview.adblockplus.org/29720653/diff/29720654/includes/hero-downl... includes/hero-download.html:27: <img class="video-thumbnail" src="/img/hero-video.jpg" srcset="/img/hero-video-2x.jpg 2x" alt="{{ video-alt-text[alt text] Introduction to Adblock Plus video thumbnail }}"> On 2018/03/12 18:07:51, juliandoucette wrote: > I forgot to delete video-thumbnail.jpg Done. https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.cs... static/css/index.css:16: padding: 3em 1em 3em 1em; On 2018/03/13 09:18:02, ire wrote: > Shouldn't this just be 3em al around? > > Spec: "Below 1024px (Below Desktop breakpoint) Ensure exactly 3rem all around > section" No. I 3em on tablet until 1em on mobile. Sorry for confusion. https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.cs... static/css/index.css:16: padding: 3em 1em 3em 1em; On 2018/03/13 09:18:02, ire wrote: > NIT: Use shorthand `3em 1em` instead Done. https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.cs... static/css/index.css:19: #hero-download h1 .avoid-wrap On 2018/03/13 09:18:03, ire wrote: > Is there a reason you didn't use `word-wrap`/`word-break` to stop the word > breaking? I think so? My intent was to prevent breaking between specific words. AFAICT these two properties/values do not do exactly that. At least, `word-break: keep-all;` doesn't seem to do it. https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.cs... static/css/index.css:52: padding: 6em 1em 5em 1em; On 2018/03/13 09:18:02, ire wrote: > NIT: Shorthand `6em 1em 5em` Done. https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.cs... static/css/index.css:52: padding: 6em 1em 5em 1em; On 2018/03/13 09:18:03, ire wrote: > Shouldn't this be `6em 1em`? > > Spec: "Ensure exactly 6rem above and below section" Yes and no. The ~space at the bottom of the image makes it *look* like more than 6em if 6em is applied. Therefore, we decided to go with 5em on bottom to achieve the 6em *effect*/look. I'm sorry. I should have addded a comment about this. https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.cs... static/css/index.css:101: border: none; On 2018/03/13 09:18:02, ire wrote: > The adding/removing of the border causes a shift. > > Suggest: Instead of removing the border here, add a transparent border of the > same width Good catch!
Thanks Julian! Almost there. https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29720653/diff/29720654/static/css/index.cs... static/css/index.css:19: #hero-download h1 .avoid-wrap On 2018/03/13 14:23:09, juliandoucette wrote: > On 2018/03/13 09:18:03, ire wrote: > > Is there a reason you didn't use `word-wrap`/`word-break` to stop the word > > breaking? > > I think so? My intent was to prevent breaking between specific words. AFAICT > these two properties/values do not do exactly that. At least, `word-break: > keep-all;` doesn't seem to do it. Acknowledged. https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:29: @media(min-width: 1024px) SuperNIT: Inconsistent spacing between `@media` and `(` . I think we want a space between these https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:34: /* 3em bottom looks like 6em because of thumbnail contents */ If the aim is to make it look like there is equal spacing to the top and bottom of this section (which from the mockup it looks like this is the case), then 3em doesn't seem to be enough. It currently looks like there is more space to the top than to the bottom of this section. https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:39: @media (max-width: 1023px) SuperNIT: The order of these media queries is a bit confusing. I would expect the styles to go in order from smaller to bigger screens. ie.. #hero-download @media (min-width: 576px) and (max-width: 1023px) { #hero-download } @media (max-width: 1023px) { #hero-download } @media(min-width: 1024px) { #hero-download } --- i.e. place this block before the previous one https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:70: font-size: 2.4em NIT: Missing semicolon https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:82: margin-bottom: 0.5em; Suggest: Change this element to a <p> instead of a <div> so we don't need to manually add this margin
Thanks Ire! New Patchset up! https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:29: @media(min-width: 1024px) On 2018/03/14 07:39:15, ire wrote: > SuperNIT: Inconsistent spacing between `@media` and `(` . I think we want a > space between these Done. Good catch. https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:34: /* 3em bottom looks like 6em because of thumbnail contents */ On 2018/03/14 07:39:15, ire wrote: > If the aim is to make it look like there is equal spacing to the top and bottom > of this section (which from the mockup it looks like this is the case), then 3em > doesn't seem to be enough. > > It currently looks like there is more space to the top than to the bottom of > this section. 4em it is? https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:39: @media (max-width: 1023px) On 2018/03/14 07:39:15, ire wrote: > SuperNIT: The order of these media queries is a bit confusing. I would expect > the styles to go in order from smaller to bigger screens. > > ie.. > > #hero-download > > @media (min-width: 576px) and (max-width: 1023px) { #hero-download } > > @media (max-width: 1023px) { #hero-download } > > @media(min-width: 1024px) { #hero-download } > > --- > > i.e. place this block before the previous one ~Done. I have moved these around slightly to find a balance between keeping similar selectors together and ordering by size. Sorry about confusion. https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:52: /* Reduced line-height to match design without hurting accessibility */ Detail: Removed line-height from this change because I will address this issue globally separately. https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:70: font-size: 2.4em On 2018/03/14 07:39:15, ire wrote: > NIT: Missing semicolon Done. Good catch. https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:82: margin-bottom: 0.5em; On 2018/03/14 07:39:15, ire wrote: > Suggest: Change this element to a <p> instead of a <div> so we don't need to > manually add this margin I changed it from a p to a div because p has margin top and bottom and I only want margin-bottom. https://codereview.adblockplus.org/29720653/diff/29722596/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29720653/diff/29722596/static/css/index.cs... static/css/index.css:106: margin-top: 2em; NIT: Added a little more space here (personal preference)
I have diffed my last patchset incorrectly. Fixing this (and rebasing) now.
On 2018/03/14 12:52:00, juliandoucette wrote: > I have diffed my last patchset incorrectly. Fixing this (and rebasing) now. Done.
https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:34: /* 3em bottom looks like 6em because of thumbnail contents */ On 2018/03/14 12:46:31, juliandoucette wrote: > On 2018/03/14 07:39:15, ire wrote: > > If the aim is to make it look like there is equal spacing to the top and > bottom > > of this section (which from the mockup it looks like this is the case), then > 3em > > doesn't seem to be enough. > > > > It currently looks like there is more space to the top than to the bottom of > > this section. > > 4em it is? I think something changed because now only 6em looks equal :/ Does 4em look equal to you? https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:39: @media (max-width: 1023px) On 2018/03/14 12:46:30, juliandoucette wrote: > On 2018/03/14 07:39:15, ire wrote: > > SuperNIT: The order of these media queries is a bit confusing. I would expect > > the styles to go in order from smaller to bigger screens. > > > > ie.. > > > > #hero-download > > > > @media (min-width: 576px) and (max-width: 1023px) { #hero-download } > > > > @media (max-width: 1023px) { #hero-download } > > > > @media(min-width: 1024px) { #hero-download } > > > > --- > > > > i.e. place this block before the previous one > > ~Done. > > I have moved these around slightly to find a balance between keeping similar > selectors together and ordering by size. > > Sorry about confusion. Thanks https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:82: margin-bottom: 0.5em; On 2018/03/14 12:46:31, juliandoucette wrote: > On 2018/03/14 07:39:15, ire wrote: > > Suggest: Change this element to a <p> instead of a <div> so we don't need to > > manually add this margin > > I changed it from a p to a div because p has margin top and bottom and I only > want margin-bottom. Ack. Because of collapsible margins the top margin doesn't have any effect here. https://codereview.adblockplus.org/29720653/diff/29722609/includes/hero-downl... File includes/hero-download.html (right): https://codereview.adblockplus.org/29720653/diff/29722609/includes/hero-downl... includes/hero-download.html:14: <a This link colour is showing up as red for me. https://codereview.adblockplus.org/29720653/diff/29722609/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29720653/diff/29722609/static/css/index.cs... static/css/index.css:52: /* 3em bottom looks like 6em because of thumbnail contents */ Comment still says 3em
New Patchset up :) https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:34: /* 3em bottom looks like 6em because of thumbnail contents */ On 2018/03/14 13:27:30, ire wrote: > I think something changed because now only 6em looks equal :/ > > Does 4em look equal to you? Good eye. I removed the font-size change. As a result, the download button is lower. Compare the bottom of the laptop (before the reflection) to the top of the laptop. (And/or apply or review my font-size change so that I can rebase on top of it.) https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:82: margin-bottom: 0.5em; On 2018/03/14 13:27:30, ire wrote: > Ack. Because of collapsible margins the top margin doesn't have any effect here. ~Ack. I could change the div.small to p.small and leave div.button and then change p.margin-bottom to 0.5em to achieve the same effect. If I change both divs to `p`s then I'd have to cancel out 1em top and bottom. I did that in my last changeset in order to take your suggestion. https://codereview.adblockplus.org/29720653/diff/29722609/includes/hero-downl... File includes/hero-download.html (right): https://codereview.adblockplus.org/29720653/diff/29722609/includes/hero-downl... includes/hero-download.html:14: <a On 2018/03/14 13:27:30, ire wrote: > This link colour is showing up as red for me. Done. Interesting. I think that is correct (based on the CSS), but I don't see it in Chrome OS X. https://codereview.adblockplus.org/29720653/diff/29722609/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29720653/diff/29722609/static/css/index.cs... static/css/index.css:52: /* 3em bottom looks like 6em because of thumbnail contents */ On 2018/03/14 13:27:30, ire wrote: > Comment still says 3em Done. Oops :D
I'll pose this question to Jeen https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29720653/diff/29721675/static/css/index.cs... static/css/index.css:34: /* 3em bottom looks like 6em because of thumbnail contents */ On 2018/03/14 14:13:41, juliandoucette wrote: > Good eye. I removed the font-size change. As a result, the download button is > lower. Compare the bottom of the laptop (before the reflection) to the top of > the laptop. > > (And/or apply or review my font-size change so that I can rebase on top of it.) It's worth considering that this text may be longer in different languages and this may look uneven as a result. I guess the question is which side would we like to optimize for? The laptop or the text?
On 2018/03/14 14:17:11, juliandoucette wrote: > I'll pose this question to Jeen > > It's worth considering that this text may be longer in different languages and > this may look uneven as a result. I guess the question is which side would we > like to optimize for? The laptop or the text? See https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/30#note_63246102
On 2018/03/14 14:29:36, juliandoucette wrote: > On 2018/03/14 14:17:11, juliandoucette wrote: > > I'll pose this question to Jeen > > > > It's worth considering that this text may be longer in different languages and > > this may look uneven as a result. I guess the question is which side would we > > like to optimize for? The laptop or the text? > > See https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/30#note_63246102 She replied here https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/30#note_63249380 New Patchset up.
On 2018/03/14 14:52:06, juliandoucette wrote: > On 2018/03/14 14:29:36, juliandoucette wrote: > > On 2018/03/14 14:17:11, juliandoucette wrote: > > > I'll pose this question to Jeen > > > > > > It's worth considering that this text may be longer in different languages > and > > > this may look uneven as a result. I guess the question is which side would > we > > > like to optimize for? The laptop or the text? > > > > See > https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/30#note_63246102 > > She replied here > https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/30#note_63249380 > > New Patchset up. Ah alright. I thought you meant you would address this for this case only then globally after. LGTM
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
