Issue 29673627: Issue 4797 - Separate white box and text from eyeo.com homepage banner (Closed)
|
Created:
Jan. 18, 2018, 4:41 p.m. by ire Modified:
Jan. 30, 2018, 7:21 a.m. Reviewers:
juliandoucette Base URL:
https://hg.adblockplus.org/web.eyeo.com Visibility:
Public. |
DescriptionIssue 4797 - Separate white box and text from eyeo.com homepage banner
Patch Set 1 #
Total comments: 23
Patch Set 2 : Optimise images #Patch Set 3 : Change images to jpg #Patch Set 4 : Use header element for #hero #
MessagesTotal messages: 10
Ready for review
It says "(Patch set is too large to download)". Maybe it will not be too large if you include fewer / optimized images? https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl File pages/index.tmpl (right): https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:17: <div> NIT: This could be a header, heading, and subtitle <header> <img> <div> <h1> <p> https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:18: <img alt="eyeo office" src="/images/eyeo-home@1x.jpg" srcset="/images/eyeo-home@1,25x.jpg 1.25x, /images/eyeo-home@2x.jpg 2x" /> You included more images than you linked to in srcset.
On 2018/01/18 21:43:51, juliandoucette wrote: > It says "(Patch set is too large to download)". Maybe it will not be too large > if you include fewer / optimized images? I updated this to only include 1x and 2x images (both optimised) but I'm still seeing the "(Patch set is too large to download)" message :/ :/ https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl File pages/index.tmpl (right): https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:17: <div> On 2018/01/18 21:43:51, juliandoucette wrote: > NIT: This could be a header, heading, and subtitle > > <header> > <img> > <div> > <h1> > <p> I considered this but I didn't really think that this was the page header. Seems more like a tagline or something rather than an <h1> https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:18: <img alt="eyeo office" src="/images/eyeo-home@1x.jpg" srcset="/images/eyeo-home@1,25x.jpg 1.25x, /images/eyeo-home@2x.jpg 2x" /> On 2018/01/18 21:43:51, juliandoucette wrote: > You included more images than you linked to in srcset. Done.
> I updated this to only include 1x and 2x images (both optimised) but I'm still > seeing the "(Patch set is too large to download)" message :/ :/ I blame Rietveld :D. Will you make sure that you aren't including hidden files by mistake and try re-uploading? If that doesn't work, will you try uploading to a separate issue (review, not trac issue)? And, if that doesn't work, will you try uploading without images (and sending them to me separately)? https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl File pages/index.tmpl (right): https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:17: <div> On 2018/01/19 09:16:27, ire wrote: > I considered this but I didn't really think that this was the page header. Seems > more like a tagline or something rather than an <h1> Seems like a header and tagline to me. Why does it not seem that way to you? https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:18: <img alt="eyeo office" src="/images/eyeo-home@1x.jpg" srcset="/images/eyeo-home@1,25x.jpg 1.25x, /images/eyeo-home@2x.jpg 2x" /> We have not used these conventions before: 1. "@Nx" (the @ sign) 2. "N,NN" (the comma) I have used "-" instead of "@" and "." instead of ",". I am open to changing these conventions and I think we should be consistent. (I'm asking you to change it or request that we change the conventions.)
On 2018/01/22 16:06:01, juliandoucette wrote: > > I updated this to only include 1x and 2x images (both optimised) but I'm still > > seeing the "(Patch set is too large to download)" message :/ :/ > > I blame Rietveld :D. > > Will you make sure that you aren't including hidden files by mistake and try > re-uploading? If that doesn't work, will you try uploading to a separate issue > (review, not trac issue)? And, if that doesn't work, will you try uploading > without images (and sending them to me separately)? I changed to jpg, significantly smaller file size ¯\_(ツ)_/¯ https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl File pages/index.tmpl (right): https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:17: <div> On 2018/01/22 16:06:01, juliandoucette wrote: > On 2018/01/19 09:16:27, ire wrote: > > I considered this but I didn't really think that this was the page header. > Seems > > more like a tagline or something rather than an <h1> > > Seems like a header and tagline to me. Why does it not seem that way to you? To clarify, I said the text seems like a tagline rather than a heading (i.e. <h1>..<h6>). I'm not necessarily arguing against the use of the <header> element. However, the use of the <header> element typically implies than an actual heading (i.e. <h1>..<h6>) will be within. My position is that I don't think the text "We want to make the internet better for everyone. Purging bad ads is a good start" is heading text. It seems to me to be a tagline, so a paragraph element that may go beneath an actual heading. I think the heading (<h1>) of this page should be "About Us" or something similar since it's the title of the document. Also, this is probably an unconscious opinion I've had, but I don't think headings (i.e. <h1>..<h6>) are generally sentences. But that might not be a real requirement. https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:18: <img alt="eyeo office" src="/images/eyeo-home@1x.jpg" srcset="/images/eyeo-home@1,25x.jpg 1.25x, /images/eyeo-home@2x.jpg 2x" /> On 2018/01/22 16:06:01, juliandoucette wrote: > We have not used these conventions before: > > 1. "@Nx" (the @ sign) > 2. "N,NN" (the comma) > > I have used "-" instead of "@" and "." instead of ",". I am open to changing > these conventions and I think we should be consistent. > > (I'm asking you to change it or request that we change the conventions.) If we were starting from scratch I would say that the @N,Nx format is cleaner, particularly when there are decimals involved. However, in this case I'll go with the -Nx format since 1) I'm not using decimals here so the advantage isn't relevant 2) I don't think the @N,Nx format is *that* much more superior, if I feel more strongly about it I'll bring it up to change it in the future
> I changed to jpg, significantly smaller file size ¯\_(ツ)_/¯ That makes sense :D https://codereview.adblockplus.org/29673627/diff/29673628/includes/index/styl... File includes/index/style.html (right): https://codereview.adblockplus.org/29673627/diff/29673628/includes/index/styl... includes/index/style.html:77: #home-image-container div NIT: It seems like #home-image-container is really more about providing a background color and the div within is really about "containing". Perhaps we should re-consider these names. e.g. <header class="bg-something"> <div class"container"> <img ...> <div id="home-header-overlay-text"> <h1>... <p>... (or similar) Detail: I used an ID for the only thing that is actually unique to this page/component. https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl File pages/index.tmpl (right): https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:17: <div> On 2018/01/25 11:54:14, ire wrote: > To clarify, I said the text seems like a tagline rather than a heading (i.e. > <h1>..<h6>). > > I'm not necessarily arguing against the use of the <header> element. However, > the use of the <header> element typically implies than an actual heading (i.e. > <h1>..<h6>) will be within. My position is that I don't think the text "We want > to make the internet better for everyone. Purging bad ads is a good start" is > heading text. It seems to me to be a tagline, so a paragraph element that may go > beneath an actual heading. > > I think the heading (<h1>) of this page should be "About Us" or something > similar since it's the title of the document. > > Also, this is probably an unconscious opinion I've had, but I don't think > headings (i.e. <h1>..<h6>) are generally sentences. But that might not be a real > requirement. I understood you. I was arguing against your interpretation of heading contents. Although it's not spelled out in the standard, heading contents (especially h1 > h2 > h3 ...) is one of the most important elements for search engine optimization. A "good" heading is typically made up of a short sentence containing as many keywords as possible without duplication or spam (AFAIK). Example: bbc.com - All of the article titles are h1s on their respective pages - All of the article titles are short (optimized?) sentences Of course, I will admit that "We want to make the internet better for everyone" is not very "optimized". And that I think it really does make a better "tagline" or "description" underneath a heading. But, perhaps, it is better than "About Us" (the unfortunate title of this page and many others on the internet - unfortunate because it's hard to compete over the keywords "about" and "us"). Therefore, I'm arguing that the proper implementation of this page consists of a header containing the <h1> "We want to make the internet better for everyone." and the subtitle <p> "Purging bad ads is a good start." - Acknowledging that we could make it all one h1 and <strong> the first part if we wanted to optimize our SEO slightly at the expense of our accessibility - Acknowledging that I would accept an sr-only h1 "eyeo" above the "tagline" that we are arguing about as a compromise between both of our solutions - But that would still not be a "good" solution IMO because it would be a wasted opportunity to put more searchable contents within the h1 on our homepage https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:18: <img alt="eyeo office" src="/images/eyeo-home@1x.jpg" srcset="/images/eyeo-home@1,25x.jpg 1.25x, /images/eyeo-home@2x.jpg 2x" /> On 2018/01/25 11:54:14, ire wrote: > If we were starting from scratch I would say that the @N,Nx format is cleaner, > particularly when there are decimals involved. However, in this case I'll go > with the -Nx format since 1) I'm not using decimals here so the advantage isn't > relevant 2) I don't think the @N,Nx format is *that* much more superior, if I > feel more strongly about it I'll bring it up to change it in the future RE: 2 - Feel free. If the "@" and "," don't have any negative side effects other than being "different" then it seems like you can/are making a good case for them (and we can change format across the board separately). https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:20: <strong>We want to make the internet better for everyone.</strong><br> - They wanted "better for everyone" on a separate line IIRC - The position of the bounding box is changed - The strong text is less strong Will you please confirm/approve these things with Jeen (or Christiane)? https://codereview.adblockplus.org/29673627/diff/29673628/static/css/styles.css File static/css/styles.css (left): https://codereview.adblockplus.org/29673627/diff/29673628/static/css/styles.c... static/css/styles.css:822: #home-image-container img Why not remove this entire selector from this file?
Thanks, new patch uploaded https://codereview.adblockplus.org/29673627/diff/29673628/includes/index/styl... File includes/index/style.html (right): https://codereview.adblockplus.org/29673627/diff/29673628/includes/index/styl... includes/index/style.html:77: #home-image-container div On 2018/01/25 14:27:29, juliandoucette wrote: > NIT: It seems like #home-image-container is really more about providing a > background color and the div within is really about "containing". Perhaps we > should re-consider these names. e.g. > > <header class="bg-something"> > <div class"container"> > <img ...> > <div id="home-header-overlay-text"> > <h1>... > <p>... > > (or similar) > > Detail: I used an ID for the only thing that is actually unique to this > page/component. Done. I kept the ID on the <header> because I need to style more than jus the overlay text https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl File pages/index.tmpl (right): https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:17: <div> On 2018/01/25 14:27:29, juliandoucette wrote: > On 2018/01/25 11:54:14, ire wrote: > > To clarify, I said the text seems like a tagline rather than a heading (i.e. > > <h1>..<h6>). > > > > I'm not necessarily arguing against the use of the <header> element. However, > > the use of the <header> element typically implies than an actual heading (i.e. > > <h1>..<h6>) will be within. My position is that I don't think the text "We > want > > to make the internet better for everyone. Purging bad ads is a good start" is > > heading text. It seems to me to be a tagline, so a paragraph element that may > go > > beneath an actual heading. > > > > I think the heading (<h1>) of this page should be "About Us" or something > > similar since it's the title of the document. > > > > Also, this is probably an unconscious opinion I've had, but I don't think > > headings (i.e. <h1>..<h6>) are generally sentences. But that might not be a > real > > requirement. > > I understood you. I was arguing against your interpretation of heading contents. > > > Although it's not spelled out in the standard, heading contents (especially h1 > > h2 > h3 ...) is one of the most important elements for search engine > optimization. A "good" heading is typically made up of a short sentence > containing as many keywords as possible without duplication or spam (AFAIK). > > Example: http://bbc.com > > - All of the article titles are h1s on their respective pages > - All of the article titles are short (optimized?) sentences > > Of course, I will admit that "We want to make the internet better for everyone" > is not very "optimized". And that I think it really does make a better "tagline" > or "description" underneath a heading. But, perhaps, it is better than "About > Us" (the unfortunate title of this page and many others on the internet - > unfortunate because it's hard to compete over the keywords "about" and "us"). > > Therefore, I'm arguing that the proper implementation of this page consists of a > header containing the <h1> "We want to make the internet better for everyone." > and the subtitle <p> "Purging bad ads is a good start." > > - Acknowledging that we could make it all one h1 and <strong> the first part if > we wanted to optimize our SEO slightly at the expense of our accessibility > - Acknowledging that I would accept an sr-only h1 "eyeo" above the "tagline" > that we are arguing about as a compromise between both of our solutions > - But that would still not be a "good" solution IMO because it would be a > wasted opportunity to put more searchable contents within the h1 on our homepage Okay. I'm going with making the whole phrase the <h1>. Why do you think this is compromising the accessibility? https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:18: <img alt="eyeo office" src="/images/eyeo-home@1x.jpg" srcset="/images/eyeo-home@1,25x.jpg 1.25x, /images/eyeo-home@2x.jpg 2x" /> On 2018/01/25 14:27:29, juliandoucette wrote: > On 2018/01/25 11:54:14, ire wrote: > > If we were starting from scratch I would say that the @N,Nx format is cleaner, > > particularly when there are decimals involved. However, in this case I'll go > > with the -Nx format since 1) I'm not using decimals here so the advantage > isn't > > relevant 2) I don't think the @N,Nx format is *that* much more superior, if I > > feel more strongly about it I'll bring it up to change it in the future > > RE: 2 - Feel free. If the "@" and "," don't have any negative side effects other > than being "different" then it seems like you can/are making a good case for > them (and we can change format across the board separately). Cool, agreed. https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:20: <strong>We want to make the internet better for everyone.</strong><br> On 2018/01/25 14:27:29, juliandoucette wrote: > - They wanted "better for everyone" on a separate line IIRC > - The position of the bounding box is changed > - The strong text is less strong > > Will you please confirm/approve these things with Jeen (or Christiane)? I showed Jeen a demo of how it currently is and she have her approval of all 3 changes. https://codereview.adblockplus.org/29673627/diff/29673628/static/css/styles.css File static/css/styles.css (left): https://codereview.adblockplus.org/29673627/diff/29673628/static/css/styles.c... static/css/styles.css:822: #home-image-container img On 2018/01/25 14:27:29, juliandoucette wrote: > Why not remove this entire selector from this file? Done.
LGTM + NIT (please consider comments before pushing). https://codereview.adblockplus.org/29673627/diff/29673628/includes/index/styl... File includes/index/style.html (right): https://codereview.adblockplus.org/29673627/diff/29673628/includes/index/styl... includes/index/style.html:77: #home-image-container div On 2018/01/26 10:11:01, ire wrote: > On 2018/01/25 14:27:29, juliandoucette wrote: > > NIT: It seems like #home-image-container is really more about providing a > > background color and the div within is really about "containing". Perhaps we > > should re-consider these names. e.g. > > > > <header class="bg-something"> > > <div class"container"> > > <img ...> > > <div id="home-header-overlay-text"> > > <h1>... > > <p>... > > > > (or similar) > > > > Detail: I used an ID for the only thing that is actually unique to this > > page/component. > > Done. I kept the ID on the <header> because I need to style more than jus the > overlay text Acknowledged. https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl File pages/index.tmpl (right): https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:17: <div> On 2018/01/26 10:11:00, ire wrote: > Okay. I'm going with making the whole phrase the <h1>. > > Why do you think this is compromising the accessibility? I don't; on second thought. I think <strong> is a reasonable implementation/compromise. My first thought went something like this: W3 recommends that we do sub-(titles/headings)/bylines like this[1]. These are clearly two sentences. One seems header like and the other seems byline like to me. But, this byline is more searchable than this header - so it would be nice to include it in the header for the sake of SEO in-case bylines in <header> elements are not given as much importance by search engines (I don't actually know). But I don't want to risk making these less semantically distinguishable as separate and title/subtitle like to screen readers. [1]: https://www.w3.org/TR/html51/sections.html#the-header-element https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:20: <strong>We want to make the internet better for everyone.</strong><br> On 2018/01/26 10:11:00, ire wrote: > I showed Jeen a demo of how it currently is and she have her approval of all 3 > changes. NIT: Did you also happen to show her in-between sizes e.g. by demo and not screenshots?
https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl File pages/index.tmpl (right): https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:17: <div> On 2018/01/30 04:22:19, juliandoucette wrote: > On 2018/01/26 10:11:00, ire wrote: > > Okay. I'm going with making the whole phrase the <h1>. > > > > Why do you think this is compromising the accessibility? > > I don't; on second thought. I think <strong> is a reasonable > implementation/compromise. > > My first thought went something like this: W3 recommends that we do > sub-(titles/headings)/bylines like this[1]. These are clearly two sentences. One > seems header like and the other seems byline like to me. But, this byline is > more searchable than this header - so it would be nice to include it in the > header for the sake of SEO in-case bylines in <header> elements are not given as > much importance by search engines (I don't actually know). But I don't want to > risk making these less semantically distinguishable as separate and > title/subtitle like to screen readers. > > [1]: https://www.w3.org/TR/html51/sections.html#the-header-element Acknowledged. https://codereview.adblockplus.org/29673627/diff/29673628/pages/index.tmpl#ne... pages/index.tmpl:20: <strong>We want to make the internet better for everyone.</strong><br> On 2018/01/30 04:22:19, juliandoucette wrote: > On 2018/01/26 10:11:00, ire wrote: > > I showed Jeen a demo of how it currently is and she have her approval of all 3 > > changes. > > NIT: Did you also happen to show her in-between sizes e.g. by demo and not > screenshots? I showed her a [screen recording](https://drive.google.com/file/d/1WXXJb2Dj-YPXmjcCmMRnFdD6KlSJXPYy/view?usp=sharing) of me resizing from desktop to mobile, so she saw all the in-between stages as well 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![src="/images/eyeo-home@1x.jpg"](mailto:src="/images/eyeo-home@1x.jpg"){kind=link}
![srcset="/images/eyeo-home@1,25x.jpg](mailto:srcset="/images/eyeo-home@1,25x.jpg){kind=link}
