Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(165)

Issue 29673627: Issue 4797 - Separate white box and text from eyeo.com homepage banner (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 8 months ago by ire
Modified:
1 year, 7 months ago
Reviewers:
juliandoucette
Base URL:
https://hg.adblockplus.org/web.eyeo.com
Visibility:
Public.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+75 lines, -16 lines) Patch
M includes/index/style.html View 1 2 3 1 chunk +57 lines, -0 lines 0 comments Download
M pages/index.tmpl View 1 2 3 1 chunk +9 lines, -3 lines 0 comments Download
M static/css/styles.css View 1 2 3 3 chunks +9 lines, -13 lines 0 comments Download
A static/images/eyeo-home-1x.jpg View 1 2 Binary file 0 comments Download
R static/images/eyeo-home-1x.png View 1 2 Binary file 0 comments Download
A static/images/eyeo-home-2x.jpg View 1 2 Binary file 0 comments Download

Messages

Total messages: 10
ire
1 year, 8 months ago (2018-01-18 16:43:00 UTC) #1
ire
Ready for review
1 year, 8 months ago (2018-01-18 16:43:52 UTC) #2
juliandoucette
It says "(Patch set is too large to download)". Maybe it will not be too ...
1 year, 8 months ago (2018-01-18 21:43:51 UTC) #3
ire
On 2018/01/18 21:43:51, juliandoucette wrote: > It says "(Patch set is too large to download)". ...
1 year, 8 months ago (2018-01-19 09:16:28 UTC) #4
juliandoucette
> I updated this to only include 1x and 2x images (both optimised) but I'm ...
1 year, 7 months ago (2018-01-22 16:06:01 UTC) #5
ire
On 2018/01/22 16:06:01, juliandoucette wrote: > > I updated this to only include 1x and ...
1 year, 7 months ago (2018-01-25 11:54:15 UTC) #6
juliandoucette
> I changed to jpg, significantly smaller file size ¯\_(ツ)_/¯ That makes sense :D https://codereview.adblockplus.org/29673627/diff/29673628/includes/index/style.html ...
1 year, 7 months ago (2018-01-25 14:27:30 UTC) #7
ire
Thanks, new patch uploaded https://codereview.adblockplus.org/29673627/diff/29673628/includes/index/style.html File includes/index/style.html (right): https://codereview.adblockplus.org/29673627/diff/29673628/includes/index/style.html#newcode77 includes/index/style.html:77: #home-image-container div On 2018/01/25 14:27:29, ...
1 year, 7 months ago (2018-01-26 10:11:01 UTC) #8
juliandoucette
LGTM + NIT (please consider comments before pushing). https://codereview.adblockplus.org/29673627/diff/29673628/includes/index/style.html File includes/index/style.html (right): https://codereview.adblockplus.org/29673627/diff/29673628/includes/index/style.html#newcode77 includes/index/style.html:77: #home-image-container ...
1 year, 7 months ago (2018-01-30 04:22:20 UTC) #9
ire
1 year, 7 months ago (2018-01-30 07:20:04 UTC) #10
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5