Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code
Issues
All Issues

Issue 29536654: Issue 5102 - Replaced semantically incorrect <hr> elements with :after styles (Closed)

Created:
Sept. 5, 2017, 2:40 p.m. by juliandoucette
Modified:
Oct. 26, 2017, 3:08 p.m.
Reviewers:
ire
Base URL:
https://hg.adblockplus.org/web.acceptableads.com
Visibility:
Public.

Description

Issue 5102 - Replaced semantically incorrect <hr> elements with :after styles

Patch Set 1 #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -47 lines) Patch
M includes/about/treating-people-with-respect.md View 1 chunk +0 lines, -2 lines 2 comments Download
M includes/criteria/general.md View 1 chunk +0 lines, -2 lines 0 comments Download
M includes/criteria/specific.md View 1 chunk +0 lines, -2 lines 0 comments Download
M includes/index/masthead.md View 1 chunk +0 lines, -3 lines 0 comments Download
M includes/tool-certification/summary.md View 1 chunk +0 lines, -2 lines 0 comments Download
M pages/about/best-practices.md View 1 chunk +0 lines, -2 lines 0 comments Download
M pages/about/index.html View 1 chunk +0 lines, -1 line 0 comments Download
M pages/blog/index.md View 1 chunk +0 lines, -2 lines 0 comments Download
M pages/committee/apply.md View 1 chunk +0 lines, -2 lines 0 comments Download
M pages/committee/documents.md View 1 chunk +0 lines, -2 lines 0 comments Download
M pages/committee/index.html View 1 chunk +0 lines, -1 line 0 comments Download
M pages/committee/members.tmpl View 1 chunk +0 lines, -1 line 0 comments Download
M pages/get-whitelisted/index.md View 1 chunk +0 lines, -2 lines 0 comments Download
M pages/solutions/ad-networks.md View 1 chunk +0 lines, -2 lines 0 comments Download
M pages/solutions/ad-tech-suppliers.md View 1 chunk +0 lines, -2 lines 0 comments Download
M pages/solutions/advertisers.md View 1 chunk +0 lines, -2 lines 0 comments Download
M pages/solutions/index.html View 1 chunk +0 lines, -1 line 0 comments Download
M pages/solutions/publishers.md View 1 chunk +0 lines, -2 lines 0 comments Download
M pages/users.md View 1 chunk +0 lines, -2 lines 0 comments Download
M static/css/main.css View 1 chunk +14 lines, -6 lines 0 comments Download
M static/scss/content/_typography.scss View 1 chunk +12 lines, -5 lines 10 comments Download
M templates/blog-entry.tmpl View 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 3
juliandoucette
Sept. 5, 2017, 2:40 p.m. (2017-09-05 14:40:21 UTC) #1
ire
Thanks Julian. Comments below. https://codereview.adblockplus.org/29536654/diff/29536655/includes/about/treating-people-with-respect.md File includes/about/treating-people-with-respect.md (left): https://codereview.adblockplus.org/29536654/diff/29536655/includes/about/treating-people-with-respect.md#oldcode4 includes/about/treating-people-with-respect.md:4: > --- The <hr> is ...
Sept. 7, 2017, 9:20 a.m. (2017-09-07 09:20:18 UTC) #2
juliandoucette
Sept. 11, 2017, 4:24 p.m. (2017-09-11 16:24:15 UTC) #3 
Thanks Ire! It looks like I was in too much of a rush with this one.

https://codereview.adblockplus.org/29536654/diff/29536655/includes/about/trea...
File includes/about/treating-people-with-respect.md (left):

https://codereview.adblockplus.org/29536654/diff/29536655/includes/about/trea...
includes/about/treating-people-with-respect.md:4: > ---
On 2017/09/07 09:20:17, ire wrote:
> The <hr> is removed, but the underline isn't replaced with CSS

Acknowledged.

This was lazy/opinionated. I'll fix it or address it separately.

https://codereview.adblockplus.org/29536654/diff/29536655/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29536654/diff/29536655/static/scss/content...
static/scss/content/_typography.scss:419: hr,
On 2017/09/07 09:20:17, ire wrote:
> NIT: We aren't using the hr element anywhere else. Maybe we should remove the
> selector?

Acknowledged.

I should have checked.

https://codereview.adblockplus.org/29536654/diff/29536655/static/scss/content...
static/scss/content/_typography.scss:431: h2 + hr,
On 2017/09/07 09:20:18, ire wrote:
> The h2 + hr selector isn't relevant anymore

Acknowledged.

I should have checked.

https://codereview.adblockplus.org/29536654/diff/29536655/static/scss/content...
static/scss/content/_typography.scss:432: h1:after
On 2017/09/07 09:20:18, ire wrote:
> This should be `.outer h1:after`. Also, all three of these selectors targeting
> `h1:after` could be grouped and conflicting styles resolved (e.g. the margins)

Acknowledged.

If you are correct about hr and h2 above; yes.

https://codereview.adblockplus.org/29536654/diff/29536655/static/scss/content...
static/scss/content/_typography.scss:446: margin-top: 32px;
On 2017/09/07 09:20:18, ire wrote:
> This style seems to be overriding the mobile style above and messing up the
> spacing on mobile

Good catch.

https://codereview.adblockplus.org/29536654/diff/29536655/static/scss/content...
static/scss/content/_typography.scss:450: .center hr
On 2017/09/07 09:20:18, ire wrote:
> NIT: See comment above. I don't think this is relevant anymore

Acknowledged.

Powered by Google App Engine
This is Rietveld