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

Issue 29361647: Issue 4607 - Default content styles (Closed)

Created:
Nov. 3, 2016, 2:11 p.m. by juliandoucette
Modified:
Dec. 13, 2016, 4:33 p.m.
Reviewers:
saroyanm
CC:
Thomas Greiner, Felix Dahlke, Philip Hill
Visibility:
Public.

Description

=== Background === We would like to define a subset of HTML5 that is also supported by markdown that we recommend using in websites content and covering in website styleguides. We will provide sensible default styles for these elements in the form of a component.

Patch Set 1 #

Patch Set 2 : Fixed content TOC and capitilization #

Patch Set 3 : Removed missing SCSS dependency #

Patch Set 4 : Changed heading and space sizes. #

Patch Set 5 : Removed missing grid dependency. #

Total comments: 93

Patch Set 6 : Addressed comments (round 1) #

Total comments: 1

Patch Set 7 : Removed accidental default in license header #

Patch Set 8 : Removed 'styleguide' from title and heading #

Patch Set 9 : Added container & simplified html/scss #

Total comments: 68

Patch Set 10 : See comments for details. #

Total comments: 14

Patch Set 11 : Removed responsive headings and unnessisary comments #

Total comments: 49

Patch Set 12 : Addressed comments, removed unnessisary components, simplified content #

Patch Set 13 : Removed extra list items and changed image text capitilization #

Total comments: 47

Patch Set 14 : See comments for details. #

Total comments: 32

Patch Set 15 : Minor change to default colors #

Total comments: 1

Patch Set 16 : See comments for details #

Unified diffs Side-by-side diffs Delta from patch set Stats (+650 lines, -0 lines) Patch
A .hgignore View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +2 lines, -0 lines 0 comments Download
A README.md View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +16 lines, -0 lines 0 comments Download
A demo/content.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +102 lines, -0 lines 0 comments Download
A demo/css/demo.css View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +24 lines, -0 lines 0 comments Download
A demo/index.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +42 lines, -0 lines 0 comments Download
A gulpfile.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +42 lines, -0 lines 0 comments Download
A package.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +15 lines, -0 lines 0 comments Download
A scss/_base.scss View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +97 lines, -0 lines 0 comments Download
A scss/_content.scss View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +198 lines, -0 lines 0 comments Download
A scss/_variables.scss View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +91 lines, -0 lines 0 comments Download
A scss/main.scss View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +21 lines, -0 lines 0 comments Download

Messages

Total messages: 36
juliandoucette
Nov. 3, 2016, 2:11 p.m. (2016-11-03 14:11:53 UTC) #1
saroyanm
Getting an Error that settings.ini is missing.
Nov. 3, 2016, 3:10 p.m. (2016-11-03 15:10:17 UTC) #2
saroyanm
I'll review "gulpfile.js" and "package.json" separately soon. Also we need to have an issue or ...
Nov. 3, 2016, 7:41 p.m. (2016-11-03 19:41:15 UTC) #3
juliandoucette
Thanks Manvel :-) We'll get on the description tomorrow. https://codereview.adblockplus.org/29361647/diff/29361698/README.md File README.md (right): https://codereview.adblockplus.org/29361647/diff/29361698/README.md#newcode5 README.md:5: ...
Nov. 3, 2016, 10:59 p.m. (2016-11-03 22:59:39 UTC) #4
juliandoucette
A few additional notes. https://codereview.adblockplus.org/29361647/diff/29361698/package.json File package.json (right): https://codereview.adblockplus.org/29361647/diff/29361698/package.json#newcode12 package.json:12: "url": "https://github.com/eyeo_gmbh/universal-design-language" NOTE: This is ...
Nov. 3, 2016, 11:16 p.m. (2016-11-03 23:16:48 UTC) #5
saroyanm
https://codereview.adblockplus.org/29361647/diff/29361698/README.md File README.md (right): https://codereview.adblockplus.org/29361647/diff/29361698/README.md#newcode5 README.md:5: ## Usage On 2016/11/03 22:59:37, juliandoucette wrote: > On ...
Nov. 4, 2016, 4:01 p.m. (2016-11-04 16:01:18 UTC) #6
juliandoucette
Thanks Manvel :-) https://codereview.adblockplus.org/29361647/diff/29361698/README.md File README.md (right): https://codereview.adblockplus.org/29361647/diff/29361698/README.md#newcode5 README.md:5: ## Usage On 2016/11/04 16:01:17, saroyanm ...
Nov. 4, 2016, 5:32 p.m. (2016-11-04 17:32:27 UTC) #7
juliandoucette
Patch updated :-) https://codereview.adblockplus.org/29361647/diff/29361698/README.md File README.md (right): https://codereview.adblockplus.org/29361647/diff/29361698/README.md#newcode5 README.md:5: ## Usage On 2016/11/04 17:32:25, juliandoucette ...
Nov. 8, 2016, 3:52 p.m. (2016-11-08 15:52:01 UTC) #8
juliandoucette
- Added container - Refactored margin/padding to use space variables and px - Added htmlhint ...
Nov. 10, 2016, 12:19 p.m. (2016-11-10 12:19:50 UTC) #9
juliandoucette
Quick note below. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#newcode72 scss/_content.scss:72: .float:after This is a mistake. ".float" ...
Nov. 10, 2016, 4:08 p.m. (2016-11-10 16:08:07 UTC) #10
saroyanm
Second round is ready, mostly contains comments about easy fixable small stuff. It's not as ...
Nov. 10, 2016, 4:30 p.m. (2016-11-10 16:30:47 UTC) #11
juliandoucette
Will submit a patch after we work out the normalize.css stuff in this review. https://codereview.adblockplus.org/29361647/diff/29362186/README.md ...
Nov. 10, 2016, 5:42 p.m. (2016-11-10 17:42:10 UTC) #12
juliandoucette
See comments for details. https://codereview.adblockplus.org/29361647/diff/29362186/README.md File README.md (right): https://codereview.adblockplus.org/29361647/diff/29362186/README.md#newcode8 README.md:8: - [gulp](http://gulpjs.com/) `npm i -g ...
Nov. 11, 2016, 4:27 p.m. (2016-11-11 16:27:20 UTC) #13
juliandoucette
One last comment. https://codereview.adblockplus.org/29361647/diff/29362471/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29361647/diff/29362471/scss/_content.scss#newcode382 scss/_content.scss:382: .container Moved mobile .container width into ...
Nov. 11, 2016, 4:32 p.m. (2016-11-11 16:32:45 UTC) #14
saroyanm
More closer now to finish CSS implementation. https://codereview.adblockplus.org/29361647/diff/29362495/package.json File package.json (right): https://codereview.adblockplus.org/29361647/diff/29362495/package.json#newcode14 package.json:14: "gulp-sourcemaps": "^2.2.0", ...
Nov. 15, 2016, 2:28 p.m. (2016-11-15 14:28:59 UTC) #15
juliandoucette
Notes from review meeting. https://codereview.adblockplus.org/29361647/diff/29362495/package.json File package.json (right): https://codereview.adblockplus.org/29361647/diff/29362495/package.json#newcode14 package.json:14: "gulp-sourcemaps": "^2.2.0", On 2016/11/15 14:28:55, ...
Nov. 16, 2016, 2:58 p.m. (2016-11-16 14:58:53 UTC) #16
juliandoucette
See additional notes on latest patchset. https://codereview.adblockplus.org/29361647/diff/29362495/gulpfile.js File gulpfile.js (right): https://codereview.adblockplus.org/29361647/diff/29362495/gulpfile.js#newcode22 gulpfile.js:22: const sourcemaps = ...
Nov. 17, 2016, 4:09 p.m. (2016-11-17 16:09:41 UTC) #17
saroyanm
We are really close I think. https://codereview.adblockplus.org/29361647/diff/29363507/gulpfile.js File gulpfile.js (right): https://codereview.adblockplus.org/29361647/diff/29363507/gulpfile.js#newcode27 gulpfile.js:27: livereload: true Suggestion: ...
Nov. 21, 2016, 6:44 p.m. (2016-11-21 18:44:42 UTC) #18
juliandoucette
Note: I updated the description to match the current requirements. In the last version I ...
Nov. 22, 2016, 12:06 a.m. (2016-11-22 00:06:16 UTC) #19
juliandoucette
See comments below. I will wait for feedback before creating a new patchset. Thanks! https://codereview.adblockplus.org/29361647/diff/29363507/gulpfile.js ...
Nov. 22, 2016, 12:54 a.m. (2016-11-22 00:54:06 UTC) #20
saroyanm
https://codereview.adblockplus.org/29361647/diff/29363507/html/content.html File html/content.html (right): https://codereview.adblockplus.org/29361647/diff/29363507/html/content.html#newcode27 html/content.html:27: <body style="margin-bottom: 3em;"> On 2016/11/22 00:54:02, juliandoucette wrote: > ...
Nov. 24, 2016, 1:26 p.m. (2016-11-24 13:26:54 UTC) #21
saroyanm
https://codereview.adblockplus.org/29361647/diff/29363507/html/content.html File html/content.html (right): https://codereview.adblockplus.org/29361647/diff/29363507/html/content.html#newcode27 html/content.html:27: <body style="margin-bottom: 3em;"> On 2016/11/22 00:54:02, juliandoucette wrote: > ...
Nov. 24, 2016, 1:26 p.m. (2016-11-24 13:26:54 UTC) #22
saroyanm
Results from the meeting https://codereview.adblockplus.org/29361647/diff/29363507/html/content.html File html/content.html (right): https://codereview.adblockplus.org/29361647/diff/29363507/html/content.html#newcode27 html/content.html:27: <body style="margin-bottom: 3em;"> On 2016/11/24 ...
Nov. 24, 2016, 3:18 p.m. (2016-11-24 15:18:45 UTC) #23
juliandoucette
See comments below. https://codereview.adblockplus.org/29361647/diff/29363507/gulpfile.js File gulpfile.js (right): https://codereview.adblockplus.org/29361647/diff/29363507/gulpfile.js#newcode26 gulpfile.js:26: root: ".", Note: I changed the ...
Nov. 25, 2016, 4:28 p.m. (2016-11-25 16:28:47 UTC) #24
juliandoucette
Updated my previous patch. See comments below for details. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#newcode229 scss/_content.scss:229: ...
Nov. 27, 2016, 9:45 p.m. (2016-11-27 21:45:42 UTC) #25
juliandoucette
https://codereview.adblockplus.org/29361647/diff/29365602/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29365602/scss/_variables.scss#newcode31 scss/_variables.scss:31: /* Brand colors Note. I made $primary-light lighter and ...
Nov. 29, 2016, 4:43 p.m. (2016-11-29 16:43:12 UTC) #26
saroyanm
https://codereview.adblockplus.org/29361647/diff/29364612/gulpfile.js File gulpfile.js (right): https://codereview.adblockplus.org/29361647/diff/29364612/gulpfile.js#newcode26 gulpfile.js:26: root: "./test/" What is the reason of naming this ...
Nov. 29, 2016, 5:51 p.m. (2016-11-29 17:51:10 UTC) #27
juliandoucette
Thanks Manvel, Feedback required. See comments below. https://codereview.adblockplus.org/29361647/diff/29364612/gulpfile.js File gulpfile.js (right): https://codereview.adblockplus.org/29361647/diff/29364612/gulpfile.js#newcode26 gulpfile.js:26: root: "./test/" ...
Nov. 29, 2016, 8:41 p.m. (2016-11-29 20:41:23 UTC) #28
juliandoucette
@Philip will you please see one question below as it relates to QA? https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html File ...
Nov. 29, 2016, 8:44 p.m. (2016-11-29 20:44:06 UTC) #29
juliandoucette
One small correction below. @Philip please see question from last email/comment. https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html File test/content.html (right): ...
Nov. 29, 2016, 8:46 p.m. (2016-11-29 20:46:42 UTC) #30
juliandoucette
Notes from review meeting. https://codereview.adblockplus.org/29361647/diff/29364612/gulpfile.js File gulpfile.js (right): https://codereview.adblockplus.org/29361647/diff/29364612/gulpfile.js#newcode26 gulpfile.js:26: root: "./test/" On 2016/11/29 20:41:21, ...
Nov. 30, 2016, 6:26 p.m. (2016-11-30 18:26:04 UTC) #31
juliandoucette
Ready for review. https://codereview.adblockplus.org/29361647/diff/29364612/gulpfile.js File gulpfile.js (right): https://codereview.adblockplus.org/29361647/diff/29364612/gulpfile.js#newcode26 gulpfile.js:26: root: "./test/" On 2016/11/30 18:26:00, juliandoucette ...
Dec. 1, 2016, 2:05 p.m. (2016-12-01 14:05:48 UTC) #32
saroyanm
I think we need to use a patches in between in case there is a ...
Dec. 1, 2016, 6:31 p.m. (2016-12-01 18:31:01 UTC) #33
juliandoucette
On 2016/12/01 18:31:01, saroyanm wrote: > I think we need to use a patches in ...
Dec. 1, 2016, 6:41 p.m. (2016-12-01 18:41:22 UTC) #34
saroyanm
On 2016/12/01 18:41:22, juliandoucette wrote: > On 2016/12/01 18:31:01, saroyanm wrote: > > I think ...
Dec. 1, 2016, 6:53 p.m. (2016-12-01 18:53:57 UTC) #35
saroyanm
Dec. 13, 2016, 3:15 p.m. (2016-12-13 15:15:14 UTC) #36
Note: I think this review can be closed

Powered by Google App Engine
This is Rietveld