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

Issue 29603821: Noissue - Separated website-default utilities

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month ago by juliandoucette
Modified:
1 hour, 55 minutes ago
Reviewers:
ire
Base URL:
https://hg.adblockplus.org/website-defaults
Visibility:
Public.

Description

Noissue - Separated website-default utilities

Patch Set 1 #

Total comments: 15

Patch Set 2 : Rebased and re-separated #

Total comments: 10

Patch Set 3 : Consolidated utilities #

Total comments: 5

Patch Set 4 : Addressed comments #13 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+194 lines, -151 lines) Patch
M static/scss/_utilities.scss View 1 2 1 chunk +0 lines, -150 lines 0 comments Download
M static/scss/main.scss View 1 2 3 1 chunk +5 lines, -1 line 0 comments Download
A static/scss/utilities/_colors.scss View 1 2 1 chunk +39 lines, -0 lines 0 comments Download
A static/scss/utilities/_layout.scss View 1 2 1 chunk +50 lines, -0 lines 0 comments Download
A static/scss/utilities/_overrides.scss View 1 2 3 1 chunk +37 lines, -0 lines 0 comments Download
A static/scss/utilities/_text.scss View 1 2 3 1 chunk +26 lines, -0 lines 0 comments Download
A static/scss/utilities/_widths.scss View 1 2 1 chunk +37 lines, -0 lines 0 comments Download

Messages

Total messages: 15
juliandoucette
1 month ago (2017-11-10 14:04:46 UTC) #1
juliandoucette
Please let em know what you think about the detail below. https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilities.scss File static/scss/_utilities.scss (right): ...
1 month ago (2017-11-10 14:08:05 UTC) #2
ire
https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilities.scss File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilities.scss#newcode18 static/scss/_utilities.scss:18: * Utilities On 2017/11/10 14:08:04, juliandoucette wrote: > Detail: ...
1 month ago (2017-11-13 08:33:40 UTC) #3
juliandoucette
https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilities.scss File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilities.scss#newcode18 static/scss/_utilities.scss:18: * Utilities On 2017/11/13 08:33:37, ire wrote: > If ...
1 month ago (2017-11-13 12:33:00 UTC) #4
ire
https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilities.scss File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilities.scss#newcode18 static/scss/_utilities.scss:18: * Utilities On 2017/11/13 12:32:59, juliandoucette wrote: > On ...
1 month ago (2017-11-14 07:14:51 UTC) #5
juliandoucette
https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utilities/_responsive-widths.scss File static/scss/utilities/_responsive-widths.scss (right): https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utilities/_responsive-widths.scss#newcode17 static/scss/utilities/_responsive-widths.scss:17: /** On 2017/11/14 07:14:51, ire wrote: > NIT: I ...
3 weeks, 6 days ago (2017-11-17 14:52:13 UTC) #6
juliandoucette
Note: It makes sense to resolve https://codereview.adblockplus.org/29612659/ before uploading a new PatchSet for this.
2 weeks, 3 days ago (2017-11-27 17:04:10 UTC) #7
ire
On 2017/11/27 17:04:10, juliandoucette wrote: > Note: It makes sense to resolve https://codereview.adblockplus.org/29612659/ > before ...
2 weeks, 2 days ago (2017-11-27 18:12:49 UTC) #8
juliandoucette
Updated.
1 week, 1 day ago (2017-12-06 15:56:05 UTC) #9
ire
Thanks, a couple of comments. https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/_utilities.scss File static/scss/_utilities.scss (left): https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/_utilities.scss#oldcode1 static/scss/_utilities.scss:1: // This file is ...
1 week ago (2017-12-07 11:44:20 UTC) #10
juliandoucette
Thanks Ire! I'll update this again today. https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/_utilities.scss File static/scss/_utilities.scss (left): https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/_utilities.scss#oldcode1 static/scss/_utilities.scss:1: // This ...
1 week ago (2017-12-07 12:33:58 UTC) #11
juliandoucette
https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/main.scss File static/scss/main.scss (right): https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/main.scss#newcode28 static/scss/main.scss:28: @import "utilities/fixed-widths.scss"; On 2017/12/07 11:44:20, ire wrote: > NIT: ...
6 days, 1 hour ago (2017-12-08 16:11:07 UTC) #12
ire
LGTM + NITs below https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/_utilities.scss File static/scss/_utilities.scss (left): https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/_utilities.scss#oldcode1 static/scss/_utilities.scss:1: // This file is part ...
3 days, 1 hour ago (2017-12-11 15:44:04 UTC) #13
juliandoucette
> It looks like it's still here. Is it actually an issue with rietveld? (Don't ...
3 hours, 37 minutes ago (2017-12-14 13:37:08 UTC) #14
ire
1 hour, 55 minutes ago (2017-12-14 15:19:17 UTC) #15
LGTM again

https://codereview.adblockplus.org/29603821/diff/29633749/static/scss/utiliti...
File static/scss/utilities/_text.scss (right):

https://codereview.adblockplus.org/29603821/diff/29633749/static/scss/utiliti...
static/scss/utilities/_text.scss:26: .unstyled,
On 2017/12/14 13:37:08, juliandoucette wrote:
> On 2017/12/11 15:44:04, ire wrote:
> > SuperNIT: I don't think .unstyled should be under the "text" category, but I
> > can't think of a better placement for now. Maybe something like "overrides"?
> We
> > can always move it later if/when we think of something better
> 
> Hard to say. I think that unstyle will usually apply to text. But it's also
> tempting to create an "overrides" file to group unstyle and sr-only. I think
> I'll do that.

+1
Sign in to reply to this message.

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