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

Issue 29596555: Noissue - Format CSS comments and implement new wd classes (Closed)

Created:
Nov. 3, 2017, 8:46 a.m. by ire
Modified:
Nov. 6, 2017, 2:41 p.m.
Reviewers:
juliandoucette
Base URL:
https://hg.adblockplus.org/help.eyeo.com
Visibility:
Public.

Description

Noissue - Format CSS comments and implement new wd classes

Patch Set 1 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+156 lines, -91 lines) Patch
M includes/contact.tmpl View 1 chunk +1 line, -1 line 0 comments Download
M static/scss/base/_utilities.scss View 2 chunks +11 lines, -30 lines 0 comments Download
M static/scss/base/_variables.scss View 2 chunks +25 lines, -20 lines 0 comments Download
M static/scss/components/_accordion.scss View 1 chunk +4 lines, -0 lines 0 comments Download
M static/scss/components/_article.scss View 1 chunk +3 lines, -1 line 0 comments Download
M static/scss/components/_breadcrumb.scss View 1 chunk +4 lines, -0 lines 0 comments Download
M static/scss/components/_browser-select.scss View 1 chunk +5 lines, -1 line 0 comments Download
M static/scss/components/_card.scss View 1 chunk +3 lines, -1 line 0 comments Download
M static/scss/components/_contact.scss View 1 chunk +4 lines, -0 lines 0 comments Download
M static/scss/components/_lists.scss View 2 chunks +18 lines, -8 lines 2 comments Download
M static/scss/components/_pre-icon.scss View 1 chunk +3 lines, -1 line 0 comments Download
M static/scss/components/_search-form.scss View 2 chunks +8 lines, -2 lines 0 comments Download
M static/scss/components/_select.scss View 2 chunks +9 lines, -3 lines 0 comments Download
M static/scss/content/_typography.scss View 2 chunks +5 lines, -1 line 0 comments Download
M static/scss/layout/_body.scss View 1 chunk +4 lines, -0 lines 0 comments Download
M static/scss/layout/_footer.scss View 1 chunk +4 lines, -0 lines 0 comments Download
M static/scss/layout/_header.scss View 2 chunks +6 lines, -1 line 0 comments Download
M static/scss/layout/_navbar.scss View 1 chunk +6 lines, -1 line 0 comments Download
M static/scss/main.scss View 1 chunk +33 lines, -20 lines 3 comments Download

Messages

Total messages: 4
ire
Nov. 3, 2017, 8:46 a.m. (2017-11-03 08:46:25 UTC) #1
ire
Ready https://codereview.adblockplus.org/29596555/diff/29596556/static/scss/main.scss File static/scss/main.scss (right): https://codereview.adblockplus.org/29596555/diff/29596556/static/scss/main.scss#newcode4 static/scss/main.scss:4: @import "base/variables"; I moved this up here because ...
Nov. 3, 2017, 8:48 a.m. (2017-11-03 08:48:31 UTC) #2
juliandoucette
LGTM + [NIT, Suggest]s https://codereview.adblockplus.org/29596555/diff/29596556/static/scss/components/_lists.scss File static/scss/components/_lists.scss (right): https://codereview.adblockplus.org/29596555/diff/29596556/static/scss/components/_lists.scss#newcode43 static/scss/components/_lists.scss:43: /* NIT: I think you ...
Nov. 6, 2017, 1:06 p.m. (2017-11-06 13:06:57 UTC) #3
ire
Nov. 6, 2017, 2:41 p.m. (2017-11-06 14:41:16 UTC) #4 
https://codereview.adblockplus.org/29596555/diff/29596556/static/scss/compone...
File static/scss/components/_lists.scss (right):

https://codereview.adblockplus.org/29596555/diff/29596556/static/scss/compone...
static/scss/components/_lists.scss:43: /*
On 2017/11/06 13:06:57, juliandoucette wrote:
> NIT: I think you can start writing on the first line

Done.

https://codereview.adblockplus.org/29596555/diff/29596556/static/scss/main.scss
File static/scss/main.scss (right):

https://codereview.adblockplus.org/29596555/diff/29596556/static/scss/main.sc...
static/scss/main.scss:4: @import "base/variables";
On 2017/11/06 13:06:57, juliandoucette wrote:
> On 2017/11/03 08:48:30, ire wrote:
> > I moved this up here because I was looking at the generated CSS file and it
> > didn't really make sense to have both the licence headers for wd and hc
> > immediately after each other. This way, all wd styles comes first, then all
hc
> > styles after. What do you think?
> 
> suggest: 
> 
> 1. Put the license header into an import
> 2. Place it below website-defaults
> 3. Put the SCSS license header back on top

Good idea. Done.

Powered by Google App Engine
This is Rietveld