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 #
MessagesTotal messages: 36
Getting an Error that settings.ini is missing.
I'll review "gulpfile.js" and "package.json" separately soon. Also we need to have an issue or at least description in the review, to make people in CC aware of the changes, also I'll suggest to add Felix into CC afterwards as well, as he is the super reviewer of websites module. 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 I think "usage" section doesn't belong to current review. https://codereview.adblockplus.org/29361647/diff/29361698/README.md#newcode5 README.md:5: ## Usage Please add the build process to the Readme file if you think it doesn't belongs here, make sense to add it to the review description. https://codereview.adblockplus.org/29361647/diff/29361698/html/content.html File html/content.html (right): https://codereview.adblockplus.org/29361647/diff/29361698/html/content.html#n... html/content.html:247: <script src="/node_modules/anchor-js/anchor.js"></script> I'll suggest not to load script that generates anchors, we need to find a better solution for anchor generation I assume, I can see that you already have anchors setup manually on this page, so I'll suggest to keep it out from current review, otherwise it gives assumption that we are planing to use it also in our websites. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:121: background-color: transparent; Detail: we do set background-color for both of them to the same value, I think this is redundant. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:162: .sol, We need to document what short classnames mean https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:173: height: auto; Nit: "auto" is default value we probably can skip it https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:175: overflow: auto; Nit: "auto" is default value we probably can skip it https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:182: float: left; Regarding using floats would be great to ask for third opinion. I'm not sure what was final decision regarding using floats. Maybe if you can provide context we can ask @Thomas to comment under it. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:203: /* Tables Note: If we are planing to use tables for tabular datas we need to be sure that we will fix responsiveness issues. I'm not sure if the table is a right element to go. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:236: .embed Why are we using this wrapper for iframe ? https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:240: height: 0; Detail: This property looks to be redundant https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:249: bottom: 0; Deatail: this property looks to be redundant https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:37: body I'm not sure if this style belongs to the reset.css https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:59: margin: $small-space 0; Detail: please specify units on all number values. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:64: margin: $small-space / 2 0; I've noticed we are using calculated margins on a lot of styles, why not to add additional variable for that reason ? https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:126: [dir="ltr"] th Are we using/planing to use Tables in our new designs ? Why do we reset only th ? https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:136: /* remove image borders lt IE 10 */ I wonder if similar changes should belong to the normalize CSS ? https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scs... scss/_variables.scss:40: $primary: $white; To what $primary variable suppose to apply ? https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scs... scss/_variables.scss:44: $inverted: $black; To what $inverted variable suppose to apply ? https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scs... scss/_variables.scss:48: $secondary: $gray-dark; To what $secondary variable suppose to apply ? https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scs... scss/_variables.scss:52: $accent: $blue; To what $accent variable suppose to apply ? https://codereview.adblockplus.org/29361647/diff/29361698/scss/main.scss File scss/main.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/main.scss#newc... scss/main.scss:19: @import "../node_modules/normalize.css/normalize"; Normalize css have a lot of styles that we probably will not use, I'll suggest to use only styles that we need. We can add more, in case needed. I can't comment on each particular style while it's not included in the review.
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: ## Usage On 2016/11/03 19:41:13, saroyanm wrote: > I think "usage" section doesn't belong to current review. Why? https://codereview.adblockplus.org/29361647/diff/29361698/README.md#newcode5 README.md:5: ## Usage On 2016/11/03 19:41:13, saroyanm wrote: > Please add the build process to the Readme file if you think it doesn't belongs > here, make sense to add it to the review description. Acknowledged. Sorry about the confusion. https://codereview.adblockplus.org/29361647/diff/29361698/html/content.html File html/content.html (right): https://codereview.adblockplus.org/29361647/diff/29361698/html/content.html#n... html/content.html:247: <script src="/node_modules/anchor-js/anchor.js"></script> On 2016/11/03 19:41:13, saroyanm wrote: > I'll suggest not to load script that generates anchors, we need to find a better > solution for anchor generation I assume, What's wrong with this solution? https://github.com/bryanbraun/anchorjs > I can see that you already have anchors setup manually on this page, I'm not sure what you mean. This script adds anchors to "header h2" on hover. > so I'll suggest to keep it out from current review, > otherwise it gives assumption that we are planing to use it also in our > websites. No problem. I decided to use this for our convenience. It was not intended to be part of our *universal styles*. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:121: background-color: transparent; On 2016/11/03 19:41:14, saroyanm wrote: > Detail: we do set background-color for both of them to the same value, I think > this is redundant. Acknowledged. Good catch. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:162: .sol, On 2016/11/03 19:41:14, saroyanm wrote: > We need to document what short classnames mean My mistake. "SOL": "Start Of Line", "EOL": "End Of Line" You are right. I should expand these to "start-of-line", "end-of-line" or document theme somewhere. Do you have a preference about how I do this? https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:173: height: auto; On 2016/11/03 19:41:13, saroyanm wrote: > Nit: "auto" is default value we probably can skip it See comment below. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:175: overflow: auto; On 2016/11/03 19:41:13, saroyanm wrote: > Nit: "auto" is default value we probably can skip it I'm not sure about this. I will investigate further. The purpose of this .block class is to clear and full width anything regardless of what is before or inside. I don't remember why I made height and overflow auto ATM. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:182: float: left; On 2016/11/03 19:41:13, saroyanm wrote: > Regarding using floats would be great to ask for third opinion. I'm not sure > what was final decision regarding using floats. > Maybe if you can provide context we can ask @Thomas to comment under it. I'm sorry. You were probably confused by the naming here. These classes are intended to be used to float images to the start-of-line or end-of-line (which is the actual purpose of floats). I could alternatively call them .float-sol and .float-eol or something to avoid confusion with text-align. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:203: /* Tables On 2016/11/03 19:41:14, saroyanm wrote: > Note: If we are planing to use tables for tabular datas we need to be sure that > we will fix responsiveness issues. I'm not sure if the table is a right element > to go. I would suggest we use simple tables for simple tabular data and address advanced features like table responsiveness, scrolling, nesting, etc, in a separate component when/if they come up. I included minimal table styles because we do have some simple tables in our websites and they are supported by markdown. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:236: .embed On 2016/11/03 19:41:14, saroyanm wrote: > Why are we using this wrapper for iframe ? See comment below. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:240: height: 0; On 2016/11/03 19:41:13, saroyanm wrote: > Detail: This property looks to be redundant See comment below. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:249: bottom: 0; On 2016/11/03 19:41:13, saroyanm wrote: > Deatail: this property looks to be redundant Sorry for the confusion. These are the same styles that were in Acceptable Ads for responsive videos. I adapted these styles from Bootstrap 4: "embed-responsive embed-responsive-16by9" -> ".embed", "embed-responsive-item" -> ".embed iframe" I trust that they crafted these styles intentionally and tested them extensively. If you are OK with using these styles then we should at least put a comment about this in the source code. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:37: body On 2016/11/03 19:41:14, saroyanm wrote: > I'm not sure if this style belongs to the reset.css Acknowledged. Where do you think it belongs? https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:59: margin: $small-space 0; On 2016/11/03 19:41:14, saroyanm wrote: > Detail: please specify units on all number values. Acknowledged. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:64: margin: $small-space / 2 0; On 2016/11/03 19:41:14, saroyanm wrote: > I've noticed we are using calculated margins on a lot of styles, why not to add > additional variable for that reason ? 1. For simplicity 2. For consistency These are supposed to be **minimal** sensible defaults. The better question is "How many sizes should we specify?" What do you think? https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:126: [dir="ltr"] th On 2016/11/03 19:41:14, saroyanm wrote: > Are we using/planing to use Tables in our new designs ? > Why do we reset only th ? Yes. We will use tables for tabular data. This is not a reset. th default text-align is center. I am setting it to left here. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:136: /* remove image borders lt IE 10 */ On 2016/11/03 19:41:14, saroyanm wrote: > I wonder if similar changes should belong to the normalize CSS ? Acknowledged. This is a mistake actually. Normalize.css seems to handle this already. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scs... scss/_variables.scss:40: $primary: $white; On 2016/11/03 19:41:15, saroyanm wrote: > To what $primary variable suppose to apply ? See comment below. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scs... scss/_variables.scss:44: $inverted: $black; On 2016/11/03 19:41:14, saroyanm wrote: > To what $inverted variable suppose to apply ? See comment below. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scs... scss/_variables.scss:48: $secondary: $gray-dark; On 2016/11/03 19:41:14, saroyanm wrote: > To what $secondary variable suppose to apply ? See comment below. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scs... scss/_variables.scss:52: $accent: $blue; On 2016/11/03 19:41:14, saroyanm wrote: > To what $accent variable suppose to apply ? Sorry for the confusion. These are common *design language* terms. I think Google explains them pretty well here: https://material.google.com/style/color.html#color-color-schemes Acceptable Ads example: Primary -> Black Secondary -> Gray Accent -> Green https://codereview.adblockplus.org/29361647/diff/29361698/scss/main.scss File scss/main.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/main.scss#newc... scss/main.scss:19: @import "../node_modules/normalize.css/normalize"; On 2016/11/03 19:41:15, saroyanm wrote: > Normalize css have a lot of styles that we probably will not use, I'll suggest > to use only styles that we need. We can add more, in case needed. > I can't comment on each particular style while it's not included in the review. I would caution us against forking a sudo-standard library like this. The point of using normalize is that we trust this library to equalize browsers and handle edge cases so that we don't have to. https://github.com/necolas/normalize.css/
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 not final. Just anticipating the future repository. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:91: color: $blue-dark; Note: This should be $accent. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:218: th NOTE: These th, tr:nth-child(even) should be combined.
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 2016/11/03 19:41:13, saroyanm wrote: > > I think "usage" section doesn't belong to current review. > > Why? Maybe because I don't understand how to use it :D I'll need to review the package and gulp first I assume. I'll return to this commit, sorry for confusion. https://codereview.adblockplus.org/29361647/diff/29361698/html/content.html File html/content.html (right): https://codereview.adblockplus.org/29361647/diff/29361698/html/content.html#n... html/content.html:247: <script src="/node_modules/anchor-js/anchor.js"></script> On 2016/11/03 22:59:37, juliandoucette wrote: > On 2016/11/03 19:41:13, saroyanm wrote: > > I'll suggest not to load script that generates anchors, we need to find a > better > > solution for anchor generation I assume, > > What's wrong with this solution? > > https://github.com/bryanbraun/anchorjs We need to check the SEO implication of deeplinks I assume, we are changing the content, which would be great to have by default. > > I can see that you already have anchors setup manually on this page, > > I'm not sure what you mean. This script adds anchors to "header h2" on hover. Sorry was confused, I thought it also creating TOC. Overlooked. > > so I'll suggest to keep it out from current review, > > otherwise it gives assumption that we are planing to use it also in our > > websites. > > No problem. I decided to use this for our convenience. It was not intended to be > part of our *universal styles*. Yes I know I think we are creating something similar in the FAQ page review for deeplinking, which is still not ideal, we should find way to add that VIA CMS I think.. Let's keep it for separate review/ticket I'd suggest https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:91: color: $blue-dark; On 2016/11/03 23:16:47, juliandoucette wrote: > Note: This should be $accent. Acknowledged. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:162: .sol, On 2016/11/03 22:59:38, juliandoucette wrote: > On 2016/11/03 19:41:14, saroyanm wrote: > > We need to document what short classnames mean > > My mistake. > > "SOL": "Start Of Line", > "EOL": "End Of Line" > > You are right. I should expand these to "start-of-line", "end-of-line" or > document theme somewhere. Do you have a preference about how I do this? I think if we use expanded version it will be in align with out style guide and self explanatory. I'd vote for expanded version. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:173: height: auto; On 2016/11/03 22:59:37, juliandoucette wrote: > On 2016/11/03 19:41:13, saroyanm wrote: > > Nit: "auto" is default value we probably can skip it > > See comment below. My comment below is wrong, but setting heigh to auto I think still actual and shouldn't be required I guess, while the initial value of height is auto https://developer.mozilla.org/en/docs/Web/CSS/height https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:175: overflow: auto; On 2016/11/03 22:59:37, juliandoucette wrote: > On 2016/11/03 19:41:13, saroyanm wrote: > > Nit: "auto" is default value we probably can skip it > > I'm not sure about this. I will investigate further. The purpose of this .block > class is to clear and full width anything regardless of what is before or > inside. I don't remember why I made height and overflow auto ATM. Scratch that - my mistake, the initial value of the overflow is "visible". https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:182: float: left; On 2016/11/03 22:59:37, juliandoucette wrote: > On 2016/11/03 19:41:13, saroyanm wrote: > > Regarding using floats would be great to ask for third opinion. I'm not sure > > what was final decision regarding using floats. > > Maybe if you can provide context we can ask @Thomas to comment under it. > > I'm sorry. You were probably confused by the naming here. These classes are > intended to be used to float images to the start-of-line or end-of-line (which > is the actual purpose of floats). > > I could alternatively call them .float-sol and .float-eol or something to avoid > confusion with text-align. Acknowledged. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:203: /* Tables On 2016/11/03 22:59:37, juliandoucette wrote: > On 2016/11/03 19:41:14, saroyanm wrote: > > Note: If we are planing to use tables for tabular datas we need to be sure > that > > we will fix responsiveness issues. I'm not sure if the table is a right > element > > to go. > > I would suggest we use simple tables for simple tabular data and address > advanced features like table responsiveness, scrolling, nesting, etc, in a > separate component when/if they come up. > > I included minimal table styles because we do have some simple tables in our > websites and they are supported by markdown. Fine with me while we are planing to make them responsive. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:218: th On 2016/11/03 23:16:47, juliandoucette wrote: > NOTE: These th, tr:nth-child(even) should be combined. Acknowledged. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:37: body On 2016/11/03 22:59:38, juliandoucette wrote: > On 2016/11/03 19:41:14, saroyanm wrote: > > I'm not sure if this style belongs to the reset.css > > Acknowledged. > > Where do you think it belongs? I think _content.scss https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:64: margin: $small-space / 2 0; On 2016/11/03 22:59:38, juliandoucette wrote: > On 2016/11/03 19:41:14, saroyanm wrote: > > I've noticed we are using calculated margins on a lot of styles, why not to > add > > additional variable for that reason ? > > 1. For simplicity > 2. For consistency > > These are supposed to be **minimal** sensible defaults. The better question is > "How many sizes should we specify?" What do you think? This makes the size dependent of the $small-space size, if we are going to use that way, fine with me. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scs... scss/_variables.scss:52: $accent: $blue; On 2016/11/03 22:59:38, juliandoucette wrote: > On 2016/11/03 19:41:14, saroyanm wrote: > > To what $accent variable suppose to apply ? > > Sorry for the confusion. These are common *design language* terms. I think > Google explains them pretty well here: > https://material.google.com/style/color.html#color-color-schemes > > Acceptable Ads example: > Primary -> Black > Secondary -> Gray > Accent -> Green Acknowledged. https://codereview.adblockplus.org/29361647/diff/29361698/scss/main.scss File scss/main.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/main.scss#newc... scss/main.scss:19: @import "../node_modules/normalize.css/normalize"; On 2016/11/03 22:59:39, juliandoucette wrote: > On 2016/11/03 19:41:15, saroyanm wrote: > > Normalize css have a lot of styles that we probably will not use, I'll suggest > > to use only styles that we need. We can add more, in case needed. > > I can't comment on each particular style while it's not included in the > review. > > I would caution us against forking a sudo-standard library like this. The point > of using normalize is that we trust this library to equalize browsers and handle > edge cases so that we don't have to. > > https://github.com/necolas/normalize.css/ It handles not only edge cases we need, but the one that we might not use at all, if we consider that logic we will end up loading bunch of CSS JS which will handle all the stuff for us. I kinda feel like that we end up fixing stuff caused by third party libraries. If you want we can ask for 3-rd opinion here.
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 wrote: > On 2016/11/03 22:59:37, juliandoucette wrote: > > On 2016/11/03 19:41:13, saroyanm wrote: > > > I think "usage" section doesn't belong to current review. > > > > Why? > > Maybe because I don't understand how to use it :D > I'll need to review the package and gulp first I assume. > I'll return to this commit, sorry for confusion. We'll leave out usage and features and add install instructions. https://codereview.adblockplus.org/29361647/diff/29361698/html/content.html File html/content.html (right): https://codereview.adblockplus.org/29361647/diff/29361698/html/content.html#n... html/content.html:247: <script src="/node_modules/anchor-js/anchor.js"></script> On 2016/11/04 16:01:17, saroyanm wrote: > On 2016/11/03 22:59:37, juliandoucette wrote: > > On 2016/11/03 19:41:13, saroyanm wrote: > > > I'll suggest not to load script that generates anchors, we need to find a > > better > > > solution for anchor generation I assume, > > > > What's wrong with this solution? > > > > https://github.com/bryanbraun/anchorjs > We need to check the SEO implication of deeplinks I assume, we are changing the > content, which would be great to have by default. > > > > I can see that you already have anchors setup manually on this page, > > > > I'm not sure what you mean. This script adds anchors to "header h2" on hover. > Sorry was confused, I thought it also creating TOC. Overlooked. > > > so I'll suggest to keep it out from current review, > > > otherwise it gives assumption that we are planing to use it also in our > > > websites. > > > > No problem. I decided to use this for our convenience. It was not intended to > be > > part of our *universal styles*. > Yes I know I think we are creating something similar in the FAQ page review for > deeplinking, which is still not ideal, we should find way to add that VIA CMS I > think.. Let's keep it for separate review/ticket I'd suggest We'll leave this out of next patchset. 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" On 2016/11/03 23:16:47, juliandoucette wrote: > NOTE: This is not final. Just anticipating the future repository. We'll leave this out for now. https://codereview.adblockplus.org/29361647/diff/29361698/package.json#newcode14 package.json:14: "dependencies": { Remove for now. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:162: .sol, On 2016/11/04 16:01:17, saroyanm wrote: > On 2016/11/03 22:59:38, juliandoucette wrote: > > On 2016/11/03 19:41:14, saroyanm wrote: > > > We need to document what short classnames mean > > > > My mistake. > > > > "SOL": "Start Of Line", > > "EOL": "End Of Line" > > > > You are right. I should expand these to "start-of-line", "end-of-line" or > > document theme somewhere. Do you have a preference about how I do this? > > I think if we use expanded version it will be in align with out style guide and > self explanatory. I'd vote for expanded version. Use: - float-start - float-end https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:173: height: auto; On 2016/11/04 16:01:17, saroyanm wrote: > On 2016/11/03 22:59:37, juliandoucette wrote: > > On 2016/11/03 19:41:13, saroyanm wrote: > > > Nit: "auto" is default value we probably can skip it > > > > See comment below. > > My comment below is wrong, but setting heigh to auto I think still actual and > shouldn't be required I guess, while the initial value of height is auto > https://developer.mozilla.org/en/docs/Web/CSS/height Acknowledged. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:175: overflow: auto; On 2016/11/04 16:01:17, saroyanm wrote: > On 2016/11/03 22:59:37, juliandoucette wrote: > > On 2016/11/03 19:41:13, saroyanm wrote: > > > Nit: "auto" is default value we probably can skip it > > > > I'm not sure about this. I will investigate further. The purpose of this > .block > > class is to clear and full width anything regardless of what is before or > > inside. I don't remember why I made height and overflow auto ATM. > > Scratch that - my mistake, the initial value of the overflow is "visible". Acknowledged. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:203: /* Tables On 2016/11/04 16:01:17, saroyanm wrote: > On 2016/11/03 22:59:37, juliandoucette wrote: > > On 2016/11/03 19:41:14, saroyanm wrote: > > > Note: If we are planing to use tables for tabular datas we need to be sure > > that > > > we will fix responsiveness issues. I'm not sure if the table is a right > > element > > > to go. > > > > I would suggest we use simple tables for simple tabular data and address > > advanced features like table responsiveness, scrolling, nesting, etc, in a > > separate component when/if they come up. > > > > I included minimal table styles because we do have some simple tables in our > > websites and they are supported by markdown. > > Fine with me while we are planing to make them responsive. We will leave out tables. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:249: bottom: 0; On 2016/11/03 22:59:37, juliandoucette wrote: > On 2016/11/03 19:41:13, saroyanm wrote: > > Deatail: this property looks to be redundant > > Sorry for the confusion. These are the same styles that were in Acceptable Ads > for responsive videos. I adapted these styles from Bootstrap 4: > > "embed-responsive embed-responsive-16by9" -> ".embed", > "embed-responsive-item" -> ".embed iframe" > > I trust that they crafted these styles intentionally and tested them > extensively. > > If you are OK with using these styles then we should at least put a comment > about this in the source code. - We can remove bottom - Add comment about how it works https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:37: body On 2016/11/04 16:01:17, saroyanm wrote: > On 2016/11/03 22:59:38, juliandoucette wrote: > > On 2016/11/03 19:41:14, saroyanm wrote: > > > I'm not sure if this style belongs to the reset.css > > > > Acknowledged. > > > > Where do you think it belongs? > > I think _content.scss Acknowledged. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:64: margin: $small-space / 2 0; On 2016/11/04 16:01:17, saroyanm wrote: > On 2016/11/03 22:59:38, juliandoucette wrote: > > On 2016/11/03 19:41:14, saroyanm wrote: > > > I've noticed we are using calculated margins on a lot of styles, why not to > > add > > > additional variable for that reason ? > > > > 1. For simplicity > > 2. For consistency > > > > These are supposed to be **minimal** sensible defaults. The better question is > > "How many sizes should we specify?" What do you think? > > This makes the size dependent of the $small-space size, if we are going to use > that way, fine with me. 3 for now. Extend it later if we need to. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:126: [dir="ltr"] th On 2016/11/03 22:59:38, juliandoucette wrote: > On 2016/11/03 19:41:14, saroyanm wrote: > > Are we using/planing to use Tables in our new designs ? > > Why do we reset only th ? > > Yes. We will use tables for tabular data. This is not a reset. th default > text-align is center. I am setting it to left here. Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scs... scss/_variables.scss:52: $accent: $blue; > Acknowledged. 1. I will add comments describing the naming scheme 2. I will make variables changable https://codereview.adblockplus.org/29361647/diff/29361698/scss/main.scss File scss/main.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/main.scss#newc... scss/main.scss:19: @import "../node_modules/normalize.css/normalize"; On 2016/11/04 16:01:18, saroyanm wrote: > On 2016/11/03 22:59:39, juliandoucette wrote: > > On 2016/11/03 19:41:15, saroyanm wrote: > > > Normalize css have a lot of styles that we probably will not use, I'll > suggest > > > to use only styles that we need. We can add more, in case needed. > > > I can't comment on each particular style while it's not included in the > > review. > > > > I would caution us against forking a sudo-standard library like this. The > point > > of using normalize is that we trust this library to equalize browsers and > handle > > edge cases so that we don't have to. > > > > https://github.com/necolas/normalize.css/ > > It handles not only edge cases we need, but the one that we might not use at > all, if we consider that logic we will end up loading bunch of CSS JS which will > handle all the stuff for us. I kinda feel like that we end up fixing stuff > caused by third party libraries. If you want we can ask for 3-rd opinion here. We'll pick and choose into _reset.scss (or whatever we name it).
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 wrote: > On 2016/11/04 16:01:17, saroyanm wrote: > > On 2016/11/03 22:59:37, juliandoucette wrote: > > > On 2016/11/03 19:41:13, saroyanm wrote: > > > > I think "usage" section doesn't belong to current review. > > > > > > Why? > > > > Maybe because I don't understand how to use it :D > > I'll need to review the package and gulp first I assume. > > I'll return to this commit, sorry for confusion. > > We'll leave out usage and features and add install instructions. Done. https://codereview.adblockplus.org/29361647/diff/29361698/html/content.html File html/content.html (right): https://codereview.adblockplus.org/29361647/diff/29361698/html/content.html#n... html/content.html:247: <script src="/node_modules/anchor-js/anchor.js"></script> On 2016/11/04 17:32:26, juliandoucette wrote: > On 2016/11/04 16:01:17, saroyanm wrote: > > On 2016/11/03 22:59:37, juliandoucette wrote: > > > On 2016/11/03 19:41:13, saroyanm wrote: > > > > I'll suggest not to load script that generates anchors, we need to find a > > > better > > > > solution for anchor generation I assume, > > > > > > What's wrong with this solution? > > > > > > https://github.com/bryanbraun/anchorjs > > We need to check the SEO implication of deeplinks I assume, we are changing > the > > content, which would be great to have by default. > > > > > > I can see that you already have anchors setup manually on this page, > > > > > > I'm not sure what you mean. This script adds anchors to "header h2" on > hover. > > Sorry was confused, I thought it also creating TOC. Overlooked. > > > > so I'll suggest to keep it out from current review, > > > > otherwise it gives assumption that we are planing to use it also in our > > > > websites. > > > > > > No problem. I decided to use this for our convenience. It was not intended > to > > be > > > part of our *universal styles*. > > Yes I know I think we are creating something similar in the FAQ page review > for > > deeplinking, which is still not ideal, we should find way to add that VIA CMS > I > > think.. Let's keep it for separate review/ticket I'd suggest > > We'll leave this out of next patchset. Done. 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" On 2016/11/04 17:32:26, juliandoucette wrote: > On 2016/11/03 23:16:47, juliandoucette wrote: > > NOTE: This is not final. Just anticipating the future repository. > > We'll leave this out for now. Done. https://codereview.adblockplus.org/29361647/diff/29361698/package.json#newcode14 package.json:14: "dependencies": { On 2016/11/04 17:32:26, juliandoucette wrote: > Remove for now. Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:91: color: $blue-dark; On 2016/11/04 16:01:17, saroyanm wrote: > On 2016/11/03 23:16:47, juliandoucette wrote: > > Note: This should be $accent. > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:121: background-color: transparent; On 2016/11/03 22:59:37, juliandoucette wrote: > On 2016/11/03 19:41:14, saroyanm wrote: > > Detail: we do set background-color for both of them to the same value, I think > > this is redundant. > > Acknowledged. > > Good catch. Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:162: .sol, On 2016/11/04 17:32:26, juliandoucette wrote: > On 2016/11/04 16:01:17, saroyanm wrote: > > On 2016/11/03 22:59:38, juliandoucette wrote: > > > On 2016/11/03 19:41:14, saroyanm wrote: > > > > We need to document what short classnames mean > > > > > > My mistake. > > > > > > "SOL": "Start Of Line", > > > "EOL": "End Of Line" > > > > > > You are right. I should expand these to "start-of-line", "end-of-line" or > > > document theme somewhere. Do you have a preference about how I do this? > > > > I think if we use expanded version it will be in align with out style guide > and > > self explanatory. I'd vote for expanded version. > > > Use: > > - float-start > - float-end Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:173: height: auto; On 2016/11/04 17:32:26, juliandoucette wrote: > On 2016/11/04 16:01:17, saroyanm wrote: > > On 2016/11/03 22:59:37, juliandoucette wrote: > > > On 2016/11/03 19:41:13, saroyanm wrote: > > > > Nit: "auto" is default value we probably can skip it > > > > > > See comment below. > > > > My comment below is wrong, but setting heigh to auto I think still actual and > > shouldn't be required I guess, while the initial value of height is auto > > https://developer.mozilla.org/en/docs/Web/CSS/height > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:175: overflow: auto; On 2016/11/04 17:32:26, juliandoucette wrote: > On 2016/11/04 16:01:17, saroyanm wrote: > > On 2016/11/03 22:59:37, juliandoucette wrote: > > > On 2016/11/03 19:41:13, saroyanm wrote: > > > > Nit: "auto" is default value we probably can skip it > > > > > > I'm not sure about this. I will investigate further. The purpose of this > > .block > > > class is to clear and full width anything regardless of what is before or > > > inside. I don't remember why I made height and overflow auto ATM. > > > > Scratch that - my mistake, the initial value of the overflow is "visible". > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:182: float: left; On 2016/11/04 16:01:17, saroyanm wrote: > On 2016/11/03 22:59:37, juliandoucette wrote: > > On 2016/11/03 19:41:13, saroyanm wrote: > > > Regarding using floats would be great to ask for third opinion. I'm not sure > > > what was final decision regarding using floats. > > > Maybe if you can provide context we can ask @Thomas to comment under it. > > > > I'm sorry. You were probably confused by the naming here. These classes are > > intended to be used to float images to the start-of-line or end-of-line (which > > is the actual purpose of floats). > > > > I could alternatively call them .float-sol and .float-eol or something to > avoid > > confusion with text-align. > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:203: /* Tables On 2016/11/04 17:32:26, juliandoucette wrote: > On 2016/11/04 16:01:17, saroyanm wrote: > > On 2016/11/03 22:59:37, juliandoucette wrote: > > > On 2016/11/03 19:41:14, saroyanm wrote: > > > > Note: If we are planing to use tables for tabular datas we need to be sure > > > that > > > > we will fix responsiveness issues. I'm not sure if the table is a right > > > element > > > > to go. > > > > > > I would suggest we use simple tables for simple tabular data and address > > > advanced features like table responsiveness, scrolling, nesting, etc, in a > > > separate component when/if they come up. > > > > > > I included minimal table styles because we do have some simple tables in our > > > websites and they are supported by markdown. > > > > Fine with me while we are planing to make them responsive. > > We will leave out tables. Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:218: th On 2016/11/04 16:01:17, saroyanm wrote: > On 2016/11/03 23:16:47, juliandoucette wrote: > > NOTE: These th, tr:nth-child(even) should be combined. > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:236: .embed On 2016/11/03 22:59:37, juliandoucette wrote: > On 2016/11/03 19:41:14, saroyanm wrote: > > Why are we using this wrapper for iframe ? > > See comment below. Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:240: height: 0; On 2016/11/03 22:59:38, juliandoucette wrote: > On 2016/11/03 19:41:13, saroyanm wrote: > > Detail: This property looks to be redundant > > See comment below. Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_content.scss#... scss/_content.scss:249: bottom: 0; On 2016/11/04 17:32:26, juliandoucette wrote: > On 2016/11/03 22:59:37, juliandoucette wrote: > > On 2016/11/03 19:41:13, saroyanm wrote: > > > Deatail: this property looks to be redundant > > > > Sorry for the confusion. These are the same styles that were in Acceptable Ads > > for responsive videos. I adapted these styles from Bootstrap 4: > > > > "embed-responsive embed-responsive-16by9" -> ".embed", > > "embed-responsive-item" -> ".embed iframe" > > > > I trust that they crafted these styles intentionally and tested them > > extensively. > > > > If you are OK with using these styles then we should at least put a comment > > about this in the source code. > > - We can remove bottom > - Add comment about how it works Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:37: body On 2016/11/04 17:32:26, juliandoucette wrote: > On 2016/11/04 16:01:17, saroyanm wrote: > > On 2016/11/03 22:59:38, juliandoucette wrote: > > > On 2016/11/03 19:41:14, saroyanm wrote: > > > > I'm not sure if this style belongs to the reset.css > > > > > > Acknowledged. > > > > > > Where do you think it belongs? > > > > I think _content.scss > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_reset.scss#ne... scss/_reset.scss:136: /* remove image borders lt IE 10 */ On 2016/11/03 22:59:38, juliandoucette wrote: > On 2016/11/03 19:41:14, saroyanm wrote: > > I wonder if similar changes should belong to the normalize CSS ? > > Acknowledged. > > This is a mistake actually. Normalize.css seems to handle this already. Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/_variables.scs... scss/_variables.scss:52: $accent: $blue; On 2016/11/04 17:32:27, juliandoucette wrote: > > Acknowledged. > > 1. I will add comments describing the naming scheme > 2. I will make variables changable Done. https://codereview.adblockplus.org/29361647/diff/29361698/scss/main.scss File scss/main.scss (right): https://codereview.adblockplus.org/29361647/diff/29361698/scss/main.scss#newc... scss/main.scss:19: @import "../node_modules/normalize.css/normalize"; On 2016/11/04 17:32:27, juliandoucette wrote: > On 2016/11/04 16:01:18, saroyanm wrote: > > On 2016/11/03 22:59:39, juliandoucette wrote: > > > On 2016/11/03 19:41:15, saroyanm wrote: > > > > Normalize css have a lot of styles that we probably will not use, I'll > > suggest > > > > to use only styles that we need. We can add more, in case needed. > > > > I can't comment on each particular style while it's not included in the > > > review. > > > > > > I would caution us against forking a sudo-standard library like this. The > > point > > > of using normalize is that we trust this library to equalize browsers and > > handle > > > edge cases so that we don't have to. > > > > > > https://github.com/necolas/normalize.css/ > > > > It handles not only edge cases we need, but the one that we might not use at > > all, if we consider that logic we will end up loading bunch of CSS JS which > will > > handle all the stuff for us. I kinda feel like that we end up fixing stuff > > caused by third party libraries. If you want we can ask for 3-rd opinion here. > > We'll pick and choose into _reset.scss (or whatever we name it). Done. https://codereview.adblockplus.org/29361647/diff/29362027/gulpfile.js File gulpfile.js (right): https://codereview.adblockplus.org/29361647/diff/29362027/gulpfile.js#newcode43 gulpfile.js:43: .pipe(sourcemaps.write("./")) Changed source maps plugin to write to external file instead of bundle (main.css.map file in /css).
- Added container - Refactored margin/padding to use space variables and px - Added htmlhint to build process - Added stylelint to build process - Removed inline styles - Changed document description
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#... scss/_content.scss:72: .float:after This is a mistake. ".float" should be ".clearfix".
Second round is ready, mostly contains comments about easy fixable small stuff. It's not as big as it might seem. 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 gulp-cli` Suggestion: don't use short version of the commands, because it's not obvious whether "i" refers to the "init" or "install": npm install --global gulp-cli Same applies to "npm install" below https://codereview.adblockplus.org/29361647/diff/29362186/package.json File package.json (right): https://codereview.adblockplus.org/29361647/diff/29362186/package.json#newcode13 package.json:13: "gulp-htmlhint": "^0.3.1", I'll suggest, not to include htmlhint and lint in current review, they are not making the review finish process easier IMO, I'll suggest to add "nice to have" features in separate reviews. https://codereview.adblockplus.org/29361647/diff/29362186/package.json#newcode16 package.json:16: "gulp-stylelint": "^3.4.0" Getting current error: No configuration provided for /_content.scss 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#... scss/_content.scss:23: * - normalize.css v5.0.0 | MIT License | github.com/necolas/normalize.css I thought we were planing to move parts of normalize.css into reset.css and and rename the file if needed. Or create separate normalize.css https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:49: line-height: 1.15; Detail: please specify units where possible. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:57: * A full-width container with small padding The way the comments are written are affecting CSS file readability IMO. I'll strongly recommend if there is a need for the comment to put it on top of the group of selectors. We can have this comments moved to HTMl file. We can have general comment on top of both, ex.: "responsive container" https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:61: width: 540px; I thought the plan was to calculate the width from the breakpoint values. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:67: .container:after, I do not think that this rules belong to current review, are they ? Looks like grid specific implementation if I'm not wrong. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:79: @media(min-width: $tablet-breakpoint) Note: I do see a reason why @media queries are located after the .container styles, but as soon we will add more rules into @media queries I think it would be more convinient to move them on the bottom of the file. Weak suggestion: we can move them to the bottom now. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:115: h1, Maybe it's a personal opinion, but I kinda feel like the overwriting the rules doesn't make the file more readable, what about removing headings from top selectors and having only here, by changing the rule to: margin: $medium-space 0px $small-space 0px; https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:164: hr Detail: In HTML "hr" element is missing. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:166: /* Add the correct box sizing in Firefox. */ What is the problem with border-box reset we have in Firefox ? https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:169: overflow: visible; Not really sure why we need overflow property on hr element ? https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:170: height: 0px; note: Feels like this style belongs to normalize.css https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:205: /* Correct odd font inheritance in all browsers. */ Detail: seems like this also part of normalization and couple of styles below I've also noticed. Make sense to decide where we want to move normalization related properties, I thought we decided to move to reset.css https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:299: .youtube-16x9 Why youtube ? I think this style shouldn't have specific website name. Previous .embed were fitting better IMO. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:304: height: 0px; I thought we decided to remove this property. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:312: bottom: 0px; I thought we decided to skip this property. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:322: abbr[title] I wonder if there will be cases when we will use abbr element without title attribute ? https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:324: /* Remove the bottom border in Firefox 39-. */ We do support 48.0+ I think make sense to only concentrate on the platforms we are supporting and add the minimal requirements in case we really need to support old versions. Reference: https://adblockplus.org/requirements https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:346: /* Prevent `sub` and `sup` elements from affecting the line height in detail: I think this comment should refer to the line below. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29361647/diff/29362186/scss/_reset.scss#ne... scss/_reset.scss:57: figcaption, Detail: I'm not really sure that some of this tags will be used (ex.: figcaption and figure), I'll suggest to keep the one that will definitely be used and add others by need. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_reset.scss#ne... scss/_reset.scss:66: [hidden] I'm not really sure if we are planing to use hidden attribute, are we ? And I think we don't need to encourage people on using the hidden attribute, it I think requires education on how to use it and when. I think we can skip this implementation for now.
Will submit a patch after we work out the normalize.css stuff in this review. 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 gulp-cli` On 2016/11/10 16:30:42, saroyanm wrote: > Suggestion: don't use short version of the commands, because it's not obvious > whether "i" refers to the "init" or "install": > npm install --global gulp-cli > Same applies to "npm install" below Acknowledged. https://codereview.adblockplus.org/29361647/diff/29362186/package.json File package.json (right): https://codereview.adblockplus.org/29361647/diff/29362186/package.json#newcode13 package.json:13: "gulp-htmlhint": "^0.3.1", On 2016/11/10 16:30:43, saroyanm wrote: > I'll suggest, not to include htmlhint and lint in current review, they are not > making the review finish process easier IMO, I'll suggest to add "nice to have" > features in separate reviews. Acknowledged. https://codereview.adblockplus.org/29361647/diff/29362186/package.json#newcode16 package.json:16: "gulp-stylelint": "^3.4.0" On 2016/11/10 16:30:42, saroyanm wrote: > Getting current error: > No configuration provided for /_content.scss Acknowledged. 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#... scss/_content.scss:23: * - normalize.css v5.0.0 | MIT License | github.com/necolas/normalize.css On 2016/11/10 16:30:44, saroyanm wrote: > I thought we were planing to move parts of normalize.css into reset.css and and > rename the file if needed. Or create separate normalize.css We split parts of normalize.css into _reset.scss and _content.scss. That's why "this file contains parts of normalize.css". We have to give attribution to comply with the MIT license. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:49: line-height: 1.15; On 2016/11/10 16:30:43, saroyanm wrote: > Detail: please specify units where possible. Acknowledged. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:57: * A full-width container with small padding On 2016/11/10 16:30:43, saroyanm wrote: > The way the comments are written are affecting CSS file readability IMO. > I'll strongly recommend if there is a need for the comment to put it on top of > the group of selectors. > We can have this comments moved to HTMl file. We can have general comment on top > of both, ex.: "responsive container" Acknowledged. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:61: width: 540px; On 2016/11/10 16:30:45, saroyanm wrote: > I thought the plan was to calculate the width from the breakpoint values. No. There is no algorithm to calculate these widths. They are based on browser statistics. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:67: .container:after, On 2016/11/10 16:30:44, saroyanm wrote: > I do not think that this rules belong to current review, are they ? Looks like > grid specific implementation if I'm not wrong. I think you are wrong. These styles clear containers and any element that contains the clearfix class. This applies to the grid and any other floating elements (EG: float-start, float-end). https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:79: @media(min-width: $tablet-breakpoint) On 2016/11/10 16:30:46, saroyanm wrote: > Note: I do see a reason why @media queries are located after the .container > styles, but as soon we will add more rules into @media queries I think it would > be more convinient to move them on the bottom of the file. Weak suggestion: we > can move them to the bottom now. Don't you think that the rules that are applied to .container are more readable if they are kept together? https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:115: h1, On 2016/11/10 16:30:46, saroyanm wrote: > Maybe it's a personal opinion, but I kinda feel like the overwriting the rules > doesn't make the file more readable, what about removing headings from top > selectors and having only here, by changing the rule to: > margin: $medium-space 0px $small-space 0px; Acknowledged. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:164: hr On 2016/11/10 16:30:46, saroyanm wrote: > Detail: In HTML "hr" element is missing. Acknowledged. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:166: /* Add the correct box sizing in Firefox. */ On 2016/11/10 16:30:45, saroyanm wrote: > What is the problem with border-box reset we have in Firefox ? Acknowledged. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:169: overflow: visible; On 2016/11/10 16:30:44, saroyanm wrote: > Not really sure why we need overflow property on hr element ? It depends on how we style hr. The default style is overflow: visible except in IE. Hence the normalize fix. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:170: height: 0px; On 2016/11/10 16:30:45, saroyanm wrote: > note: Feels like this style belongs to normalize.css I put the normalize.css things that apply to content in content.scss https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:205: /* Correct odd font inheritance in all browsers. */ On 2016/11/10 16:30:45, saroyanm wrote: > Detail: seems like this also part of normalization and couple of styles below > I've also noticed. Make sense to decide where we want to move normalization > related properties, I thought we decided to move to reset.css See comment above. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:299: .youtube-16x9 On 2016/11/10 16:30:44, saroyanm wrote: > Why youtube ? I think this style shouldn't have specific website name. Previous > .embed were fitting better IMO. Acknowledged. I thought it was simpler for content developers. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:304: height: 0px; On 2016/11/10 16:30:45, saroyanm wrote: > I thought we decided to remove this property. Acknowledged. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:312: bottom: 0px; On 2016/11/10 16:30:44, saroyanm wrote: > I thought we decided to skip this property. Acknowledged. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:322: abbr[title] On 2016/11/10 16:30:43, saroyanm wrote: > I wonder if there will be cases when we will use abbr element without title > attribute ? Acknowledged. This came from normalize. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:324: /* Remove the bottom border in Firefox 39-. */ On 2016/11/10 16:30:43, saroyanm wrote: > We do support 48.0+ I think make sense to only concentrate on the platforms we > are supporting and add the minimal requirements in case we really need to > support old versions. > Reference: https://adblockplus.org/requirements Acknowledged. I could have used this link a long time ago :D... https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:346: /* Prevent `sub` and `sup` elements from affecting the line height in On 2016/11/10 16:30:46, saroyanm wrote: > detail: I think this comment should refer to the line below. Acknowledged. It does, according to normalize.css :/
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 gulp-cli` On 2016/11/10 17:42:05, juliandoucette wrote: > On 2016/11/10 16:30:42, saroyanm wrote: > > Suggestion: don't use short version of the commands, because it's not obvious > > whether "i" refers to the "init" or "install": > > npm install --global gulp-cli > > Same applies to "npm install" below > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29362186/package.json File package.json (right): https://codereview.adblockplus.org/29361647/diff/29362186/package.json#newcode13 package.json:13: "gulp-htmlhint": "^0.3.1", On 2016/11/10 17:42:06, juliandoucette wrote: > On 2016/11/10 16:30:43, saroyanm wrote: > > I'll suggest, not to include htmlhint and lint in current review, they are not > > making the review finish process easier IMO, I'll suggest to add "nice to > have" > > features in separate reviews. > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29362186/package.json#newcode16 package.json:16: "gulp-stylelint": "^3.4.0" On 2016/11/10 17:42:05, juliandoucette wrote: > On 2016/11/10 16:30:42, saroyanm wrote: > > Getting current error: > > No configuration provided for /_content.scss > > Acknowledged. Done. 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#... scss/_content.scss:49: line-height: 1.15; On 2016/11/10 17:42:07, juliandoucette wrote: > On 2016/11/10 16:30:43, saroyanm wrote: > > Detail: please specify units where possible. > > Acknowledged. MDN prefers unit-less numbers for line-height values. @see https://developer.mozilla.org/en-US/docs/Web/CSS/line-height#Prefer_unitless_... I recommend we leave this as-is. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:57: * A full-width container with small padding On 2016/11/10 17:42:08, juliandoucette wrote: > On 2016/11/10 16:30:43, saroyanm wrote: > > The way the comments are written are affecting CSS file readability IMO. > > I'll strongly recommend if there is a need for the comment to put it on top of > > the group of selectors. > > We can have this comments moved to HTMl file. We can have general comment on > top > > of both, ex.: "responsive container" > > Acknowledged. Done. - I removed these comments (I don't think we need them). - I removed .container-fluid (I don't think we need it). https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:61: width: 540px; On 2016/11/10 17:42:07, juliandoucette wrote: > On 2016/11/10 16:30:45, saroyanm wrote: > > I thought the plan was to calculate the width from the breakpoint values. > > No. There is no algorithm to calculate these widths. They are based on browser > statistics. I have included ${device}-${width} variables in _variables.scss instead. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:67: .container:after, On 2016/11/10 17:42:09, juliandoucette wrote: > On 2016/11/10 16:30:44, saroyanm wrote: > > I do not think that this rules belong to current review, are they ? Looks like > > grid specific implementation if I'm not wrong. > > I think you are wrong. These styles clear containers and any element that > contains the clearfix class. This applies to the grid and any other floating > elements (EG: float-start, float-end). Done. I separated these classes and added more explanation. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:72: .float:after On 2016/11/10 16:08:07, juliandoucette wrote: > This is a mistake. ".float" should be ".clearfix". Done. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:79: @media(min-width: $tablet-breakpoint) On 2016/11/10 17:42:08, juliandoucette wrote: > On 2016/11/10 16:30:46, saroyanm wrote: > > Note: I do see a reason why @media queries are located after the .container > > styles, but as soon we will add more rules into @media queries I think it > would > > be more convinient to move them on the bottom of the file. Weak suggestion: we > > can move them to the bottom now. > > Don't you think that the rules that are applied to .container are more readable > if they are kept together? Done. Looks like you and Thomas outnumber me on this one. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:115: h1, On 2016/11/10 17:42:06, juliandoucette wrote: > On 2016/11/10 16:30:46, saroyanm wrote: > > Maybe it's a personal opinion, but I kinda feel like the overwriting the rules > > doesn't make the file more readable, what about removing headings from top > > selectors and having only here, by changing the rule to: > > margin: $medium-space 0px $small-space 0px; > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:164: hr On 2016/11/10 17:42:09, juliandoucette wrote: > On 2016/11/10 16:30:46, saroyanm wrote: > > Detail: In HTML "hr" element is missing. > > Acknowledged. Done. Added. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:166: /* Add the correct box sizing in Firefox. */ On 2016/11/10 17:42:06, juliandoucette wrote: > On 2016/11/10 16:30:45, saroyanm wrote: > > What is the problem with border-box reset we have in Firefox ? > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:169: overflow: visible; On 2016/11/10 17:42:10, juliandoucette wrote: > On 2016/11/10 16:30:44, saroyanm wrote: > > Not really sure why we need overflow property on hr element ? > > It depends on how we style hr. The default style is overflow: visible except in > IE. Hence the normalize fix. Done. Removed. These styles were not necessary for us. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:170: height: 0px; On 2016/11/10 17:42:09, juliandoucette wrote: > On 2016/11/10 16:30:45, saroyanm wrote: > > note: Feels like this style belongs to normalize.css > > I put the normalize.css things that apply to content in content.scss Most of the normalize.css changes are made irrelevant by our styles now. Let me know if you find anything that smells. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:205: /* Correct odd font inheritance in all browsers. */ On 2016/11/10 17:42:08, juliandoucette wrote: > On 2016/11/10 16:30:45, saroyanm wrote: > > Detail: seems like this also part of normalization and couple of styles below > > I've also noticed. Make sense to decide where we want to move normalization > > related properties, I thought we decided to move to reset.css > > See comment above. Correction: These comments are no longer valid because I changed the CSS from normalize.css. Font-family and font-size are set inconsistently across browsers. Setting them for pre and code corrects this. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:299: .youtube-16x9 On 2016/11/10 17:42:09, juliandoucette wrote: > On 2016/11/10 16:30:44, saroyanm wrote: > > Why youtube ? I think this style shouldn't have specific website name. > Previous > > .embed were fitting better IMO. > > Acknowledged. > > I thought it was simpler for content developers. Done. I changed it back to .embed and added a comment. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:304: height: 0px; On 2016/11/10 17:42:07, juliandoucette wrote: > On 2016/11/10 16:30:45, saroyanm wrote: > > I thought we decided to remove this property. > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:312: bottom: 0px; On 2016/11/10 17:42:06, juliandoucette wrote: > On 2016/11/10 16:30:44, saroyanm wrote: > > I thought we decided to skip this property. > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:322: abbr[title] On 2016/11/10 17:42:06, juliandoucette wrote: > On 2016/11/10 16:30:43, saroyanm wrote: > > I wonder if there will be cases when we will use abbr element without title > > attribute ? > > Acknowledged. > > This came from normalize. Done. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:324: /* Remove the bottom border in Firefox 39-. */ On 2016/11/10 17:42:08, juliandoucette wrote: > On 2016/11/10 16:30:43, saroyanm wrote: > > We do support 48.0+ I think make sense to only concentrate on the platforms we > > are supporting and add the minimal requirements in case we really need to > > support old versions. > > Reference: https://adblockplus.org/requirements > > Acknowledged. > > I could have used this link a long time ago :D... > Done. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_content.scss#... scss/_content.scss:346: /* Prevent `sub` and `sup` elements from affecting the line height in On 2016/11/10 17:42:08, juliandoucette wrote: > On 2016/11/10 16:30:46, saroyanm wrote: > > detail: I think this comment should refer to the line below. > > Acknowledged. > > It does, according to normalize.css :/ Done. I moved and shortened the comment. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29361647/diff/29362186/scss/_reset.scss#ne... scss/_reset.scss:57: figcaption, On 2016/11/10 16:30:46, saroyanm wrote: > Detail: I'm not really sure that some of this tags will be used (ex.: figcaption > and figure), I'll suggest to keep the one that will definitely be used and add > others by need. Done. https://codereview.adblockplus.org/29361647/diff/29362186/scss/_reset.scss#ne... scss/_reset.scss:66: [hidden] On 2016/11/10 16:30:47, saroyanm wrote: > I'm not really sure if we are planing to use hidden attribute, are we ? And I > think we don't need to encourage people on using the hidden attribute, it I > think requires education on how to use it and when. > I think we can skip this implementation for now. Done. https://codereview.adblockplus.org/29361647/diff/29362471/html/content.html File html/content.html (right): https://codereview.adblockplus.org/29361647/diff/29362471/html/content.html#n... html/content.html:28: <div class="container"> No need for `main` when *everything* is main content. https://codereview.adblockplus.org/29361647/diff/29362471/html/content.html#n... html/content.html:33: <h2>Headings</h2> Adjusted HTML outline via heading rank. https://codereview.adblockplus.org/29361647/diff/29362471/html/content.html#n... html/content.html:42: <h2>Body content</h2> Combined several sections into "Body content" for simplicity. https://codereview.adblockplus.org/29361647/diff/29362471/html/content.html#n... html/content.html:50: <p>This has a footnote. <sup><a href="#" class="footnote-ref">1</a></sup></p> Added footnote-ref class that is generated by our CMS. https://codereview.adblockplus.org/29361647/diff/29362471/html/content.html#n... html/content.html:88: <h3>Unstyled lists</h3> Plural for consistency. https://codereview.adblockplus.org/29361647/diff/29362471/html/content.html#n... html/content.html:94: <ul class="unstyled-list"> See comment in _content.scss regarding .unstyled-list https://codereview.adblockplus.org/29361647/diff/29362471/html/content.html#n... html/content.html:144: <img class="full-width" src="//placehold.it/992x140?text=Block"> See comment in _content.scss regarding full-width. 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#... scss/_content.scss:24: /* UDL content styles Combined document title and TOC. https://codereview.adblockplus.org/29361647/diff/29362471/scss/_content.scss#... scss/_content.scss:28: * - Body content Shortened TOC by removing 1-off elements/classes from the tree. https://codereview.adblockplus.org/29361647/diff/29362471/scss/_content.scss#... scss/_content.scss:46: .container Removed .container-fluid because any block element is technically a fluid container. https://codereview.adblockplus.org/29361647/diff/29362471/scss/_content.scss#... scss/_content.scss:63: .clearfix Separated clearfix because it can apply to containers with content (EG: clearing a paragraph after a floating image). https://codereview.adblockplus.org/29361647/diff/29362471/scss/_content.scss#... scss/_content.scss:80: /* Headings Adjusted heading margin and font-size to be mobile first. See non-mobile sizes below in media queries. https://codereview.adblockplus.org/29361647/diff/29362471/scss/_content.scss#... scss/_content.scss:331: .full-width Changed .block class back to .full-width for simplicity.
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#... scss/_content.scss:382: .container Moved mobile .container width into media query for consistency + readability.
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", Trailing comma throws an error when I try to install dependencies. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:53: .container:after Why are we clearing the container ? It's not float as far as I can see. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:146: top: -0.5em; Why are we using EMs here ? I assume this change is for "Normalization" reasons ? https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:148: /* Prevent `sub` and `sup` elements from affecting the line height */ Detail: It's only sup in this case, you can override both in reset CSS maybe ? https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:150: vertical-align: baseline; Why are we aligning to the baseline ? Doesn't looks like it provides any value. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:167: /* Set default color and decoration (opinionated) */ * I can't see default color being set. * I'm not sure if we need this comment at all/ * I think we can align with designers to style hover state to at least make it obvious that users are hovering a link (can be done separately from this review) https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:290: audio, I think we only need this styles in case we will host videos/audios on our side, but whhile we are using 3-rd party websites that ex. youtube which we are loading in the Iframe this styles do not have an effect. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:328: clear: both; Why are we clearing the full width elements ? I thought we are planing to control the clearing behavior on the floating elements ex. setting up :after element, so I can't see a reason of using the clear on the elements that suppose to be just a full width elements, or am I missing smth ? https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:329: overflow: auto; Why do we need to set the overflow value here ? I can't see a reason why the default value is not enough for this element ? https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:374: @media(min-width: $mobile-breakpoint) Seems like we started to support 4 different screen sizes in comparison to old 3: 1. Is this change intentional ? 2. I think if we support 4 sizes we need to change the naming, because $mobile-breakpoint and $desktop-breakpoint behave inconsistently IMO. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scs... scss/_variables.scss:28: /* Basic colors Do we need this ? I thought people will use brand colors and adjust them according to the website/project. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scs... scss/_variables.scss:54: $primary: $white !default; Do we have a process of overriding the default variables ? I think if we will only use this file to change the variables we do not need to setup default values ? How you imagine the override process and why are we using defaults here ? https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scs... scss/_variables.scss:79: $sans-font: sans-serif !default; Detail: having variable with the same names as the font itself doesn't make much sense I think, we need to understand where the font will be used to make the naming consistent. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scs... scss/_variables.scss:80: $serif-font: serif !default; I can't see this font being used in content.
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, saroyanm wrote: > Trailing comma throws an error when I try to install dependencies. Acknowledged. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:53: .container:after On 2016/11/15 14:28:56, saroyanm wrote: > Why are we clearing the container ? It's not float as far as I can see. Acknowledged. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:63: .clearfix NOTE: We will drop this class and re-add if needed. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:146: top: -0.5em; On 2016/11/15 14:28:55, saroyanm wrote: > Why are we using EMs here ? > I assume this change is for "Normalization" reasons ? Acknowledged. We will remove this in favor of vertical-align: super. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:148: /* Prevent `sub` and `sup` elements from affecting the line height */ On 2016/11/15 14:28:57, saroyanm wrote: > Detail: It's only sup in this case, you can override both in reset CSS maybe ? Acknowledged. We probably don't need line-height here. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:150: vertical-align: baseline; On 2016/11/15 14:28:56, saroyanm wrote: > Why are we aligning to the baseline ? Doesn't looks like it provides any value. Acknowledged. We will change this to vertical-align: super. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:167: /* Set default color and decoration (opinionated) */ On 2016/11/15 14:28:55, saroyanm wrote: > * I can't see default color being set. > * I'm not sure if we need this comment at all/ > * I think we can align with designers to style hover state to at least make it > obvious that users are hovering a link (can be done separately from this review) Acknowledged. I will remove and/or adjust the comment. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:290: audio, On 2016/11/15 14:28:57, saroyanm wrote: > I think we only need this styles in case we will host videos/audios on our side, > but whhile we are using 3-rd party websites that ex. youtube which we are > loading in the Iframe this styles do not have an effect. Acknowledged. Agreed. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:328: clear: both; On 2016/11/15 14:28:56, saroyanm wrote: > Why are we clearing the full width elements ? > I thought we are planing to control the clearing behavior on the floating > elements ex. setting up :after element, so I can't see a reason of using the > clear on the elements that suppose to be just a full width elements, or am I > missing smth ? Acknowledged. We can handle this edge case with inline CSS or clearfix depending on how often we run into it. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:329: overflow: auto; On 2016/11/15 14:28:55, saroyanm wrote: > Why do we need to set the overflow value here ? > I can't see a reason why the default value is not enough for this element ? Acknowledged. This addresses an edge case where we put floating elements inside of a full-width container. We will address this edge case individually instead of in UDL. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:374: @media(min-width: $mobile-breakpoint) On 2016/11/15 14:28:56, saroyanm wrote: > Seems like we started to support 4 different screen sizes in comparison to old > 3: > 1. Is this change intentional ? > 2. I think if we support 4 sizes we need to change the naming, because > $mobile-breakpoint and $desktop-breakpoint behave inconsistently IMO. Acknowledged. We will remove mobile-breakpoint and keep mobile-width. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scs... scss/_variables.scss:28: /* Basic colors On 2016/11/15 14:28:57, saroyanm wrote: > Do we need this ? I thought people will use brand colors and adjust them > according to the website/project. Acknowledged. We will create variations of brand colors as-needed instead. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scs... scss/_variables.scss:54: $primary: $white !default; On 2016/11/15 14:28:58, saroyanm wrote: > Do we have a process of overriding the default variables ? I think if we will > only use this file to change the variables we do not need to setup default > values ? How you imagine the override process and why are we using defaults here > ? Acknowledged. We will add a DO NOT EDIT comment in this file explaining how SCSS variables work. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scs... scss/_variables.scss:79: $sans-font: sans-serif !default; On 2016/11/15 14:28:58, saroyanm wrote: > Detail: having variable with the same names as the font itself doesn't make much > sense I think, we need to understand where the font will be used to make the > naming consistent. Acknowledged. We will change these to primary/secondary instead of sans/serif. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scs... scss/_variables.scss:80: $serif-font: serif !default; On 2016/11/15 14:28:58, saroyanm wrote: > I can't see this font being used in content. Acknowledged.
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 = require("gulp-sourcemaps"); Note: I removed sourcemaps because I don't think they are necessary unless we are minifying CSS (which we are not currently). https://codereview.adblockplus.org/29361647/diff/29362495/html/content.html File html/content.html (right): https://codereview.adblockplus.org/29361647/diff/29362495/html/content.html#n... html/content.html:113: <h3>Code blocks</h3> Note: I removed code and pre > code from content because I think they should be a component (with syntax highlighting). https://codereview.adblockplus.org/29361647/diff/29362495/html/content.html#n... html/content.html:133: <h3>Embedded video</h3> Note: I removed embedded video because we don't have many of them on our websites. I think it's better to make this a simple component. https://codereview.adblockplus.org/29361647/diff/29362495/html/content.html#n... html/content.html:139: <h2>Image alignment</h2> Note: I removed floating images because they complicate clearing and responsiveness. (I don't want to encourage content developers to use them.) 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/16 14:58:48, juliandoucette wrote: > On 2016/11/15 14:28:55, saroyanm wrote: > > Trailing comma throws an error when I try to install dependencies. > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:53: .container:after On 2016/11/16 14:58:51, juliandoucette wrote: > On 2016/11/15 14:28:56, saroyanm wrote: > > Why are we clearing the container ? It's not float as far as I can see. > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:63: .clearfix On 2016/11/16 14:58:48, juliandoucette wrote: > NOTE: We will drop this class and re-add if needed. Done. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:146: top: -0.5em; On 2016/11/16 14:58:50, juliandoucette wrote: > On 2016/11/15 14:28:55, saroyanm wrote: > > Why are we using EMs here ? > > I assume this change is for "Normalization" reasons ? > > Acknowledged. > > We will remove this in favor of vertical-align: super. Done. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:148: /* Prevent `sub` and `sup` elements from affecting the line height */ On 2016/11/16 14:58:50, juliandoucette wrote: > On 2016/11/15 14:28:57, saroyanm wrote: > > Detail: It's only sup in this case, you can override both in reset CSS maybe ? > > Acknowledged. > > We probably don't need line-height here. Done. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:150: vertical-align: baseline; On 2016/11/16 14:58:48, juliandoucette wrote: > On 2016/11/15 14:28:56, saroyanm wrote: > > Why are we aligning to the baseline ? Doesn't looks like it provides any > value. > > Acknowledged. > > We will change this to vertical-align: super. Done. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:167: /* Set default color and decoration (opinionated) */ On 2016/11/16 14:58:50, juliandoucette wrote: > On 2016/11/15 14:28:55, saroyanm wrote: > > * I can't see default color being set. > > * I'm not sure if we need this comment at all/ > > * I think we can align with designers to style hover state to at least make it > > obvious that users are hovering a link (can be done separately from this > review) > > Acknowledged. > > I will remove and/or adjust the comment. Done. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:290: audio, On 2016/11/16 14:58:49, juliandoucette wrote: > On 2016/11/15 14:28:57, saroyanm wrote: > > I think we only need this styles in case we will host videos/audios on our > side, > > but whhile we are using 3-rd party websites that ex. youtube which we are > > loading in the Iframe this styles do not have an effect. > > Acknowledged. > > Agreed. Done. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:328: clear: both; On 2016/11/16 14:58:49, juliandoucette wrote: > On 2016/11/15 14:28:56, saroyanm wrote: > > Why are we clearing the full width elements ? > > I thought we are planing to control the clearing behavior on the floating > > elements ex. setting up :after element, so I can't see a reason of using the > > clear on the elements that suppose to be just a full width elements, or am I > > missing smth ? > > Acknowledged. > > We can handle this edge case with inline CSS or clearfix depending on how often > we run into it. Done. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:329: overflow: auto; On 2016/11/16 14:58:49, juliandoucette wrote: > On 2016/11/15 14:28:55, saroyanm wrote: > > Why do we need to set the overflow value here ? > > I can't see a reason why the default value is not enough for this element ? > > Acknowledged. > > This addresses an edge case where we put floating elements inside of a > full-width container. We will address this edge case individually instead of in > UDL. Done. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_content.scss#... scss/_content.scss:374: @media(min-width: $mobile-breakpoint) On 2016/11/16 14:58:50, juliandoucette wrote: > On 2016/11/15 14:28:56, saroyanm wrote: > > Seems like we started to support 4 different screen sizes in comparison to old > > 3: > > 1. Is this change intentional ? > > 2. I think if we support 4 sizes we need to change the naming, because > > $mobile-breakpoint and $desktop-breakpoint behave inconsistently IMO. > > Acknowledged. > > We will remove mobile-breakpoint and keep mobile-width. Done. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scs... scss/_variables.scss:28: /* Basic colors On 2016/11/16 14:58:52, juliandoucette wrote: > On 2016/11/15 14:28:57, saroyanm wrote: > > Do we need this ? I thought people will use brand colors and adjust them > > according to the website/project. > > Acknowledged. > > We will create variations of brand colors as-needed instead. Done. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scs... scss/_variables.scss:54: $primary: $white !default; On 2016/11/16 14:58:51, juliandoucette wrote: > On 2016/11/15 14:28:58, saroyanm wrote: > > Do we have a process of overriding the default variables ? I think if we will > > only use this file to change the variables we do not need to setup default > > values ? How you imagine the override process and why are we using defaults > here > > ? > > Acknowledged. > > We will add a DO NOT EDIT comment in this file explaining how SCSS variables > work. Done. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scs... scss/_variables.scss:79: $sans-font: sans-serif !default; On 2016/11/16 14:58:51, juliandoucette wrote: > On 2016/11/15 14:28:58, saroyanm wrote: > > Detail: having variable with the same names as the font itself doesn't make > much > > sense I think, we need to understand where the font will be used to make the > > naming consistent. > > Acknowledged. > > We will change these to primary/secondary instead of sans/serif. Done. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scs... scss/_variables.scss:101: $mobile-breakpoint: 576px !default; NOTE: Removed mobile breakpoint. https://codereview.adblockplus.org/29361647/diff/29362495/scss/_variables.scs... scss/_variables.scss:101: $mobile-breakpoint: 576px !default; Done.
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: I'd vote for removing this feature, I think it's just a nice sugar, but it makes almost impossible comparison of two tabs, which sometimes can be useful. https://codereview.adblockplus.org/29361647/diff/29363507/gulpfile.js#newcode31 gulpfile.js:31: gulp.task("html", function() What is this task used for ? Do we need it ? 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#n... html/content.html:27: <body style="margin-bottom: 3em;"> Detail: let's not encourage people to use inline styles 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#... scss/_content.scss:48: @media(min-width: $tablet-breakpoint) I thought we decided to move this rules to the bottom: https://codereview.adblockplus.org/29361647/diff2/29362186:29363507/scss/_con... https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:52: width: $tablet-width; What about calculating container width according to the breakpoint value ? I thought we agreed to do so. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:90: font-size: round($large-font * 1.6); I would avoid using SASS methods(like round) until it's really needed. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:90: font-size: round($large-font * 1.6); Thought: I think you already mentioned that and I want to stress that topic again, what you think if we will start using more accessible measures (like ems) for the fonts-sizes as you suggested while ago ? That I assume may also solve the problem of multiplying and using round methods here I think. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:123: text-decoration: underline dotted; Apparently shorthand properties are only supported in Firefox and Safari -> https://developer.mozilla.org/en/docs/Web/CSS/text-decoration https://codereview.adblockplus.org/29361647/diff/29363507/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29363507/scss/_variables.scs... scss/_variables.scss:85: /* Container widths Not sure if we need this variables, see my comment here: https://codereview.adblockplus.org/29361647/diff2/29362495:29363507/scss/_con...
Note: I updated the description to match the current requirements. In the last version I dropped: - inline code - code blocks - embedded video - floating images Because these rarely appear in our websites and they may be implemented on a single page or with more features (EG: Syntax highlighting) when they appear. (I also do not want to encourage our content developers to use floating or inline images because it creates responsive design edge cases.)
See comments below. I will wait for feedback before creating a new patchset. Thanks! 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 On 2016/11/21 18:44:35, saroyanm wrote: > Suggestion: I'd vote for removing this feature, I think it's just a nice sugar, > but it makes almost impossible comparison of two tabs, which sometimes can be > useful. Acknowledged. I will remove it. https://codereview.adblockplus.org/29361647/diff/29363507/gulpfile.js#newcode31 gulpfile.js:31: gulp.task("html", function() On 2016/11/21 18:44:35, saroyanm wrote: > What is this task used for ? > Do we need it ? Acknowledged. It's used to reload the page when HTML changes. If we remove livereload then there is no need for this task. 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#n... html/content.html:27: <body style="margin-bottom: 3em;"> On 2016/11/21 18:44:35, saroyanm wrote: > Detail: let's not encourage people to use inline styles Do we really want to create a new <style> for *just* one rule though :/ ? I'm OK with inline styles for spacing on specific elements to cover edge cases. 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#... scss/_content.scss:27: /* Global @Manvel can you also acknowledge and review the requirements to verify that I am not forgetting something that should / shouldn't be in this component? https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:34: line-height: 1.15; Note: I think I should set the primary foreground and background colors here. Thoughts @Manvel? https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:40: .container I'm considering moving this to _reset.scss and renaming _reset.scss to _base.scss because there are few things being "reset" in _reset.scss and a "container" is not "content". Thoughts @Manvel? https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:48: @media(min-width: $tablet-breakpoint) On 2016/11/21 18:44:39, saroyanm wrote: > I thought we decided to move this rules to the bottom: > https://codereview.adblockplus.org/29361647/diff2/29362186:29363507/scss/_con... Acknowledged. I shouldn't have moved this back. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:52: width: $tablet-width; On 2016/11/21 18:44:40, saroyanm wrote: > What about calculating container width according to the breakpoint value ? I > thought we agreed to do so. No. I said before that: - The breakpoints are exact numbers based on common device sizes - The widths are rounded numbers that fit comfortably within the breakpoint sizes How would we calculate the widths without using SCSS methods like round (which we are trying to avoid?) or subtracting very specific numbers (which are invalidated if we change the breakpoints)? https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:90: font-size: round($large-font * 1.6); On 2016/11/21 18:44:40, saroyanm wrote: > Thought: I think you already mentioned that and I want to stress that topic > again, what you think if we will start using more accessible measures (like ems) > for the fonts-sizes as you suggested while ago ? > That I assume may also solve the problem of multiplying and using round methods > here I think. The idea here was to base the larger font sizes on $large-font size. This is not possible if we want to avoid using round(). I recommend using rem and a polyfill (for IE8 only, like we do already for media queries, HTML5 tags, and other JavaScript features on some websites): https://github.com/chuckcarpenter/REM-unit-polyfill https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:123: text-decoration: underline dotted; On 2016/11/21 18:44:36, saroyanm wrote: > Apparently shorthand properties are only supported in Firefox and Safari -> > https://developer.mozilla.org/en/docs/Web/CSS/text-decoration Acknowledged. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29363507/scss/_variables.scs... scss/_variables.scss:85: /* Container widths On 2016/11/21 18:44:41, saroyanm wrote: > Not sure if we need this variables, see my comment here: > https://codereview.adblockplus.org/29361647/diff2/29362495:29363507/scss/_con... Acknowledged. See my comment on the other page.
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#n... html/content.html:27: <body style="margin-bottom: 3em;"> On 2016/11/22 00:54:02, juliandoucette wrote: > On 2016/11/21 18:44:35, saroyanm wrote: > > Detail: let's not encourage people to use inline styles > > Do we really want to create a new <style> for *just* one rule though :/ ? > > I'm OK with inline styles for spacing on specific elements to cover edge cases. I see couple of reasons: * Overwriting inline-styles can become problem * It's easier to find same selectors/styles in style tag to move to the main style in case they are being repeated. * In theory we shouldn't have edge cases to cover by inline-styles * You use it once, non-developers will use them everywhere So yes, I would vote for creating rules instead of assigning styles directly to elements. 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#... scss/_content.scss:27: /* Global On 2016/11/22 00:54:04, juliandoucette wrote: > @Manvel can you also acknowledge and review the requirements to verify that I am > not forgetting something that should / shouldn't be in this component? I've noticed that the ticket is out of sync, ex. we removed pre and code blocks, considering to implement them in future. For the initial push I think this file looks good, we can add more styles as soon we will align with designers, I think they also will review it as soon we will have this ready. Long story short: Let's move forward and add components as soon we will need them in current file. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:34: line-height: 1.15; On 2016/11/22 00:54:02, juliandoucette wrote: > Note: I think I should set the primary foreground and background colors here. > > Thoughts @Manvel? Agree. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:40: .container On 2016/11/22 00:54:04, juliandoucette wrote: > I'm considering moving this to _reset.scss and renaming _reset.scss to > _base.scss because there are few things being "reset" in _reset.scss and a > "container" is not "content". > > Thoughts @Manvel? I think we might need in future separate file for reset and normalization, but I'm fine with it, make sense to separate them in the file. I'll be fine also having reset and base separately. Don't feel strong regarding both solutions. Also make sense to move ".full-width" class I think and "html" selector. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:52: width: $tablet-width; On 2016/11/22 00:54:03, juliandoucette wrote: > On 2016/11/21 18:44:40, saroyanm wrote: > > What about calculating container width according to the breakpoint value ? I > > thought we agreed to do so. > > No. > > I said before that: > > - The breakpoints are exact numbers based on common device sizes > - The widths are rounded numbers that fit comfortably within the breakpoint > sizes > > How would we calculate the widths without using SCSS methods like round (which > we are trying to avoid?) or subtracting very specific numbers (which are > invalidated if we change the breakpoints)? I was suggestion something like -> "$tablet-breakpoint - 48px" https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:90: font-size: round($large-font * 1.6); On 2016/11/22 00:54:03, juliandoucette wrote: > On 2016/11/21 18:44:40, saroyanm wrote: > > Thought: I think you already mentioned that and I want to stress that topic > > again, what you think if we will start using more accessible measures (like > ems) > > for the fonts-sizes as you suggested while ago ? > > That I assume may also solve the problem of multiplying and using round > methods > > here I think. > The idea here was to base the larger font sizes on $large-font size. This is not > possible if we want to avoid using round(). What about having the floating pixel values ? Anyway if we will use relative approach we will not need the rounding anymore. > I recommend using rem and a polyfill (for IE8 only, like we do already for media > queries, HTML5 tags, and other JavaScript features on some websites): > https://github.com/chuckcarpenter/REM-unit-polyfill * REMs are not completely supported till IE11 and apparently some limitation for Chrome for Android. * It mostly depends on the approach why you think we need REMs ? They act differently and mostly depends on the approach we are trying to propose. * With rems you can't change the font-size of child elements relatively by assigning font-size to the parent. * I think EMs provide more flexibility in this case My suggestion was about improving accessibility and for that reason Even if we will use 1 of two approaches we need to remove fixed font-size from HTML element.
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#n... html/content.html:27: <body style="margin-bottom: 3em;"> On 2016/11/22 00:54:02, juliandoucette wrote: > On 2016/11/21 18:44:35, saroyanm wrote: > > Detail: let's not encourage people to use inline styles > > Do we really want to create a new <style> for *just* one rule though :/ ? > > I'm OK with inline styles for spacing on specific elements to cover edge cases. I see couple of reasons: * Overwriting inline-styles can become problem * It's easier to find same selectors/styles in style tag to move to the main style in case they are being repeated. * In theory we shouldn't have edge cases to cover by inline-styles * You use it once, non-developers will use them everywhere So yes, I would vote for creating rules instead of assigning styles directly to elements. 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#... scss/_content.scss:27: /* Global On 2016/11/22 00:54:04, juliandoucette wrote: > @Manvel can you also acknowledge and review the requirements to verify that I am > not forgetting something that should / shouldn't be in this component? I've noticed that the ticket is out of sync, ex. we removed pre and code blocks, considering to implement them in future. For the initial push I think this file looks good, we can add more styles as soon we will align with designers, I think they also will review it as soon we will have this ready. Long story short: Let's move forward and add components as soon we will need them in current file. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:34: line-height: 1.15; On 2016/11/22 00:54:02, juliandoucette wrote: > Note: I think I should set the primary foreground and background colors here. > > Thoughts @Manvel? Agree. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:40: .container On 2016/11/22 00:54:04, juliandoucette wrote: > I'm considering moving this to _reset.scss and renaming _reset.scss to > _base.scss because there are few things being "reset" in _reset.scss and a > "container" is not "content". > > Thoughts @Manvel? I think we might need in future separate file for reset and normalization, but I'm fine with it, make sense to separate them in the file. I'll be fine also having reset and base separately. Don't feel strong regarding both solutions. Also make sense to move ".full-width" class I think and "html" selector. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:52: width: $tablet-width; On 2016/11/22 00:54:03, juliandoucette wrote: > On 2016/11/21 18:44:40, saroyanm wrote: > > What about calculating container width according to the breakpoint value ? I > > thought we agreed to do so. > > No. > > I said before that: > > - The breakpoints are exact numbers based on common device sizes > - The widths are rounded numbers that fit comfortably within the breakpoint > sizes > > How would we calculate the widths without using SCSS methods like round (which > we are trying to avoid?) or subtracting very specific numbers (which are > invalidated if we change the breakpoints)? I was suggestion something like -> "$tablet-breakpoint - 48px" https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:90: font-size: round($large-font * 1.6); On 2016/11/22 00:54:03, juliandoucette wrote: > On 2016/11/21 18:44:40, saroyanm wrote: > > Thought: I think you already mentioned that and I want to stress that topic > > again, what you think if we will start using more accessible measures (like > ems) > > for the fonts-sizes as you suggested while ago ? > > That I assume may also solve the problem of multiplying and using round > methods > > here I think. > The idea here was to base the larger font sizes on $large-font size. This is not > possible if we want to avoid using round(). What about having the floating pixel values ? Anyway if we will use relative approach we will not need the rounding anymore. > I recommend using rem and a polyfill (for IE8 only, like we do already for media > queries, HTML5 tags, and other JavaScript features on some websites): > https://github.com/chuckcarpenter/REM-unit-polyfill * REMs are not completely supported till IE11 and apparently some limitation for Chrome for Android. * It mostly depends on the approach why you think we need REMs ? They act differently and mostly depends on the approach we are trying to propose. * With rems you can't change the font-size of child elements relatively by assigning font-size to the parent. * I think EMs provide more flexibility in this case My suggestion was about improving accessibility and for that reason Even if we will use 1 of two approaches we need to remove fixed font-size from HTML element.
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#n... html/content.html:27: <body style="margin-bottom: 3em;"> On 2016/11/24 13:26:49, saroyanm wrote: > On 2016/11/22 00:54:02, juliandoucette wrote: > > On 2016/11/21 18:44:35, saroyanm wrote: > > > Detail: let's not encourage people to use inline styles > > > > Do we really want to create a new <style> for *just* one rule though :/ ? > > > > I'm OK with inline styles for spacing on specific elements to cover edge > cases. > > I see couple of reasons: > * Overwriting inline-styles can become problem > * It's easier to find same selectors/styles in style tag to move to the main > style in case they are being repeated. > * In theory we shouldn't have edge cases to cover by inline-styles > * You use it once, non-developers will use them everywhere > > So yes, I would vote for creating rules instead of assigning styles directly to > elements. Acknowledged. We gonna use <style> tag 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#... scss/_content.scss:27: /* Global On 2016/11/24 13:26:51, saroyanm wrote: > On 2016/11/22 00:54:04, juliandoucette wrote: > > @Manvel can you also acknowledge and review the requirements to verify that I > am > > not forgetting something that should / shouldn't be in this component? > > I've noticed that the ticket is out of sync, ex. we removed pre and code blocks, > considering to implement them in future. > For the initial push I think this file looks good, we can add more styles as > soon we will align with designers, I think they also will review it as soon we > will have this ready. > Long story short: Let's move forward and add components as soon we will need > them in current file. Acknowledged. We will move forward and add more component by need. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:40: .container On 2016/11/24 13:26:50, saroyanm wrote: > On 2016/11/22 00:54:04, juliandoucette wrote: > > I'm considering moving this to _reset.scss and renaming _reset.scss to > > _base.scss because there are few things being "reset" in _reset.scss and a > > "container" is not "content". > > > > Thoughts @Manvel? > > I think we might need in future separate file for reset and normalization, but > I'm fine with it, make sense to separate them in the file. I'll be fine also > having reset and base separately. > Don't feel strong regarding both solutions. > Also make sense to move ".full-width" class I think and "html" selector. Acknowledged. We will leave here only "markdown" related rules and move reset/normalization related general and base rules to the _base.scss. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:52: width: $tablet-width; On 2016/11/24 13:26:50, saroyanm wrote: > On 2016/11/22 00:54:03, juliandoucette wrote: > > On 2016/11/21 18:44:40, saroyanm wrote: > > > What about calculating container width according to the breakpoint value ? I > > > thought we agreed to do so. > > > > No. > > > > I said before that: > > > > - The breakpoints are exact numbers based on common device sizes > > - The widths are rounded numbers that fit comfortably within the breakpoint > > sizes > > > > How would we calculate the widths without using SCSS methods like round (which > > we are trying to avoid?) or subtracting very specific numbers (which are > > invalidated if we change the breakpoints)? > > I was suggestion something like -> "$tablet-breakpoint - 48px" Acknowledged. We will keep variables and not calculate according to breakpoints [designers need to be on charge of changing them] https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:90: font-size: round($large-font * 1.6); On 2016/11/24 13:26:49, saroyanm wrote: > On 2016/11/22 00:54:03, juliandoucette wrote: > > On 2016/11/21 18:44:40, saroyanm wrote: > > > Thought: I think you already mentioned that and I want to stress that topic > > > again, what you think if we will start using more accessible measures (like > > ems) > > > for the fonts-sizes as you suggested while ago ? > > > That I assume may also solve the problem of multiplying and using round > > methods > > > here I think. > > The idea here was to base the larger font sizes on $large-font size. This is > not > > possible if we want to avoid using round(). > What about having the floating pixel values ? Anyway if we will use relative > approach we will not need the rounding anymore. > > > I recommend using rem and a polyfill (for IE8 only, like we do already for > media > > queries, HTML5 tags, and other JavaScript features on some websites): > > https://github.com/chuckcarpenter/REM-unit-polyfill > > * REMs are not completely supported till IE11 and apparently some limitation for > Chrome for Android. > * It mostly depends on the approach why you think we need REMs ? They act > differently and mostly depends on the approach we are trying to propose. > * With rems you can't change the font-size of child elements relatively by > assigning font-size to the parent. > * I think EMs provide more flexibility in this case > > My suggestion was about improving accessibility and for that reason Even if we > will use 1 of two approaches we need to remove fixed font-size from HTML > element. Acknowledged. We will use Ems for everything content related, but keep away from the components. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29363507/scss/_variables.scs... scss/_variables.scss:82: $tablet-breakpoint: 768px !default; We will remove "!default", because people should not be able to edit this numbers.
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 root from "." to "test" (previously "html") so that you don't have to put the subdirectory in the url. https://codereview.adblockplus.org/29361647/diff/29363507/gulpfile.js#newcode27 gulpfile.js:27: livereload: true On 2016/11/22 00:54:02, juliandoucette wrote: > On 2016/11/21 18:44:35, saroyanm wrote: > > Suggestion: I'd vote for removing this feature, I think it's just a nice > sugar, > > but it makes almost impossible comparison of two tabs, which sometimes can be > > useful. > > Acknowledged. > > I will remove it. Done. https://codereview.adblockplus.org/29361647/diff/29363507/gulpfile.js#newcode31 gulpfile.js:31: gulp.task("html", function() On 2016/11/22 00:54:02, juliandoucette wrote: > On 2016/11/21 18:44:35, saroyanm wrote: > > What is this task used for ? > > Do we need it ? > > Acknowledged. > > It's used to reload the page when HTML changes. If we remove livereload then > there is no need for this task. Done. https://codereview.adblockplus.org/29361647/diff/29363507/gulpfile.js#newcode41 gulpfile.js:41: .pipe(gulp.dest("./css/")) Note: I changed this output directory so that we don't have to put subdirectories in the url. 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#n... html/content.html:27: <body style="margin-bottom: 3em;"> On 2016/11/24 15:18:42, saroyanm wrote: > On 2016/11/24 13:26:49, saroyanm wrote: > > On 2016/11/22 00:54:02, juliandoucette wrote: > > > On 2016/11/21 18:44:35, saroyanm wrote: > > > > Detail: let's not encourage people to use inline styles > > > > > > Do we really want to create a new <style> for *just* one rule though :/ ? > > > > > > I'm OK with inline styles for spacing on specific elements to cover edge > > cases. > > > > I see couple of reasons: > > * Overwriting inline-styles can become problem > > * It's easier to find same selectors/styles in style tag to move to the main > > style in case they are being repeated. > > * In theory we shouldn't have edge cases to cover by inline-styles > > * You use it once, non-developers will use them everywhere > > > > So yes, I would vote for creating rules instead of assigning styles directly > to > > elements. > > Acknowledged. > > We gonna use <style> tag Done. 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#... scss/_content.scss:27: /* Global On 2016/11/24 15:18:42, saroyanm wrote: > On 2016/11/24 13:26:51, saroyanm wrote: > > On 2016/11/22 00:54:04, juliandoucette wrote: > > > @Manvel can you also acknowledge and review the requirements to verify that > I > > am > > > not forgetting something that should / shouldn't be in this component? > > > > I've noticed that the ticket is out of sync, ex. we removed pre and code > blocks, > > considering to implement them in future. > > For the initial push I think this file looks good, we can add more styles as > > soon we will align with designers, I think they also will review it as soon we > > will have this ready. > > Long story short: Let's move forward and add components as soon we will need > > them in current file. > > Acknowledged. > > We will move forward and add more component by need. Done. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:34: line-height: 1.15; On 2016/11/24 13:26:52, saroyanm wrote: > On 2016/11/22 00:54:02, juliandoucette wrote: > > Note: I think I should set the primary foreground and background colors here. > > > > Thoughts @Manvel? > > Agree. Done. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:40: .container On 2016/11/24 15:18:44, saroyanm wrote: > On 2016/11/24 13:26:50, saroyanm wrote: > > On 2016/11/22 00:54:04, juliandoucette wrote: > > > I'm considering moving this to _reset.scss and renaming _reset.scss to > > > _base.scss because there are few things being "reset" in _reset.scss and a > > > "container" is not "content". > > > > > > Thoughts @Manvel? > > > > I think we might need in future separate file for reset and normalization, but > > I'm fine with it, make sense to separate them in the file. I'll be fine also > > having reset and base separately. > > Don't feel strong regarding both solutions. > > Also make sense to move ".full-width" class I think and "html" selector. > > Acknowledged. > > We will leave here only "markdown" related rules and move reset/normalization > related general and base rules to the _base.scss. Done. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:48: @media(min-width: $tablet-breakpoint) On 2016/11/22 00:54:04, juliandoucette wrote: > On 2016/11/21 18:44:39, saroyanm wrote: > > I thought we decided to move this rules to the bottom: > > > https://codereview.adblockplus.org/29361647/diff2/29362186:29363507/scss/_con... > > Acknowledged. > > I shouldn't have moved this back. Done. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:52: width: $tablet-width; On 2016/11/24 15:18:43, saroyanm wrote: > On 2016/11/24 13:26:50, saroyanm wrote: > > On 2016/11/22 00:54:03, juliandoucette wrote: > > > On 2016/11/21 18:44:40, saroyanm wrote: > > > > What about calculating container width according to the breakpoint value ? > I > > > > thought we agreed to do so. > > > > > > No. > > > > > > I said before that: > > > > > > - The breakpoints are exact numbers based on common device sizes > > > - The widths are rounded numbers that fit comfortably within the breakpoint > > > sizes > > > > > > How would we calculate the widths without using SCSS methods like round > (which > > > we are trying to avoid?) or subtracting very specific numbers (which are > > > invalidated if we change the breakpoints)? > > > > I was suggestion something like -> "$tablet-breakpoint - 48px" > > Acknowledged. > > We will keep variables and not calculate according to breakpoints [designers > need to be on charge of changing them] Done. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:90: font-size: round($large-font * 1.6); On 2016/11/24 15:18:43, saroyanm wrote: > On 2016/11/24 13:26:49, saroyanm wrote: > > On 2016/11/22 00:54:03, juliandoucette wrote: > > > On 2016/11/21 18:44:40, saroyanm wrote: > > > > Thought: I think you already mentioned that and I want to stress that > topic > > > > again, what you think if we will start using more accessible measures > (like > > > ems) > > > > for the fonts-sizes as you suggested while ago ? > > > > That I assume may also solve the problem of multiplying and using round > > > methods > > > > here I think. > > > The idea here was to base the larger font sizes on $large-font size. This is > > not > > > possible if we want to avoid using round(). > > What about having the floating pixel values ? Anyway if we will use relative > > approach we will not need the rounding anymore. > > > > > I recommend using rem and a polyfill (for IE8 only, like we do already for > > media > > > queries, HTML5 tags, and other JavaScript features on some websites): > > > https://github.com/chuckcarpenter/REM-unit-polyfill > > > > * REMs are not completely supported till IE11 and apparently some limitation > for > > Chrome for Android. > > * It mostly depends on the approach why you think we need REMs ? They act > > differently and mostly depends on the approach we are trying to propose. > > * With rems you can't change the font-size of child elements relatively by > > assigning font-size to the parent. > > * I think EMs provide more flexibility in this case > > > > My suggestion was about improving accessibility and for that reason Even if we > > will use 1 of two approaches we need to remove fixed font-size from HTML > > element. > > Acknowledged. > We will use Ems for everything content related, but keep away from the > components. Done. Except for line-height because of https://developer.mozilla.org/en/docs/Web/CSS/line-height#Prefer_unitless_num... https://codereview.adblockplus.org/29361647/diff/29363507/scss/_content.scss#... scss/_content.scss:123: text-decoration: underline dotted; On 2016/11/22 00:54:03, juliandoucette wrote: > On 2016/11/21 18:44:36, saroyanm wrote: > > Apparently shorthand properties are only supported in Firefox and Safari -> > > https://developer.mozilla.org/en/docs/Web/CSS/text-decoration > > Acknowledged. Done. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29363507/scss/_variables.scs... scss/_variables.scss:82: $tablet-breakpoint: 768px !default; On 2016/11/24 15:18:44, saroyanm wrote: > We will remove "!default", because people should not be able to edit this > numbers. Done. https://codereview.adblockplus.org/29361647/diff/29363507/scss/_variables.scs... scss/_variables.scss:85: /* Container widths On 2016/11/22 00:54:05, juliandoucette wrote: > On 2016/11/21 18:44:41, saroyanm wrote: > > Not sure if we need this variables, see my comment here: > > > https://codereview.adblockplus.org/29361647/diff2/29362495:29363507/scss/_con... > > Acknowledged. > > See my comment on the other page. Done. https://codereview.adblockplus.org/29361647/diff/29364584/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29361647/diff/29364584/scss/_content.scss#... scss/_content.scss:62: margin-bottom: 1rem; Note: I'm using rem here with px fallback for IE8 because I want a fixed (not em) margin-bottom that is also accessible (changes according to browser default font-size).
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#... scss/_content.scss:229: * Remove list style from vertical lists Note. Removed .unstyled-list because it cannot be applied via Markdown with markdown extensions. EG: - 1 - 2 - 3 {: .test } Applies .test to the last list item, not the list. I think we should implement this in a separate component like .embed. https://codereview.adblockplus.org/29361647/diff/29364612/README.md File README.md (right): https://codereview.adblockplus.org/29361647/diff/29364612/README.md#newcode1 README.md:1: # Website defaults Note. Renamed like we discussed. https://codereview.adblockplus.org/29361647/diff/29364612/package.json File package.json (right): https://codereview.adblockplus.org/29361647/diff/29364612/package.json#newcode9 package.json:9: ], Note. Changed to eyeo addresses because this is an eyeo project. https://codereview.adblockplus.org/29361647/diff/29364612/scss/_base.scss File scss/_base.scss (right): https://codereview.adblockplus.org/29361647/diff/29364612/scss/_base.scss#new... scss/_base.scss:23: html Note. Moved to base because it applies more to *setting up the document* then *styling the content*. https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html File test/content.html (right): https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:26: <link rel="stylesheet" href="css/demo.css"> Note. Moved inline styles into test/css/demo.css https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:32: <p>Default styles for a subset of HTML5 elements that are supported by Markdown and recommended for website content and styleguide coverage.</p> Note. Updated the description and added requirements with dumb deeplinks. https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:40: <li><a id="dir" onclick="changeDirection()">Right-to-left</a></li> Note. onclick is officially supported in HTML5. (I didn't know.) https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:96: <p> Note. Images are always surrounded by paragraphs when converted from markdown unless they appear in the middle of text. As a result, we can rely on top and bottom margins via paragraph within content. https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:104: <script src="js/demo.js"></script> Note. Created test/js/demo.js for switching the direction via the link in requirements. https://codereview.adblockplus.org/29361647/diff/29364612/test/index.html File test/index.html (right): https://codereview.adblockplus.org/29361647/diff/29364612/test/index.html#new... test/index.html:1: <!DOCTYPE html> Note. Created basic index for components. https://codereview.adblockplus.org/29361647/diff/29364612/test/js/demo.js File test/js/demo.js (right): https://codereview.adblockplus.org/29361647/diff/29364612/test/js/demo.js#new... test/js/demo.js:21: function changeDirection() Note. Seems like we could use this across components if we follow the same format (component heading, description, requirements with rtl link).
https://codereview.adblockplus.org/29361647/diff/29365602/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29365602/scss/_variables.scs... scss/_variables.scss:31: /* Brand colors Note. I made $primary-light lighter and $secondary-light darker as it makes more sense for forms.
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 as a test ? https://codereview.adblockplus.org/29361647/diff/29364612/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29364612/scss/_variables.scs... scss/_variables.scss:78: $small-space: 16px !default; Detail: we are only using $small-space currently https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html File test/content.html (right): https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:34: <h2>Requirements</h2> Detail: It's table of content not requirements. https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:40: <li><a id="dir" onclick="changeDirection()">Right-to-left</a></li> On 2016/11/27 21:45:40, juliandoucette wrote: > Note. > > onclick is officially supported in HTML5. > > (I didn't know.) Not really sure what you mean, Event attributes are discouraged to be used: https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Event_attributes#Exam... https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:41: <li><a href="http://browserl.ist/?q=last+2+versions%2C+Safari+6%2C+IE+8">Last 2 versions, Safari 6, IE 8</a></li> What about linking to -> https://adblockplus.org/requirements "last 2 versions" doesn't include dev builds of browsers.
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/" On 2016/11/29 17:51:07, saroyanm wrote: > What is the reason of naming this as a test ? Because "test" is more descriptive than "html". These pages are intended for testing / demonstrating the features of components. https://codereview.adblockplus.org/29361647/diff/29364612/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29364612/scss/_variables.scs... scss/_variables.scss:78: $small-space: 16px !default; On 2016/11/29 17:51:07, saroyanm wrote: > Detail: we are only using $small-space currently Don't you think it's a good idea to define medium and large anyway to set a precedent? - We will use them in other components - By setting them we are defining a naming format and a suggestion about how many sizes should be defined https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html File test/content.html (right): https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:34: <h2>Requirements</h2> On 2016/11/29 17:51:08, saroyanm wrote: > Detail: It's table of content not requirements. This was intended to be a high level list of requirements that also link to implementations. https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:40: <li><a id="dir" onclick="changeDirection()">Right-to-left</a></li> On 2016/11/29 17:51:07, saroyanm wrote: > On 2016/11/27 21:45:40, juliandoucette wrote: > > Note. > > > > onclick is officially supported in HTML5. > > > > (I didn't know.) > > Not really sure what you mean, Event attributes are discouraged to be used: > https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Event_attributes#Exam... Acknowledged. I meant that I think this is a good use case for event attributes, which are now standard in HTML5. My choice was: 1. Use event attributes, getElementById, and innerHTML OR 2. Polyfill addEventListener, DOMContentLoaded, querySelectorAll, and innerText (via ie8.js) -- to support IE 8. I guess we will have to include ie8.js later for other components anyway. So I will change this.
@Philip will you please see one question below as it relates to QA? https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html File test/content.html (right): https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:41: <li><a href="http://browserl.ist/?q=last+2+versions%2C+Safari+6%2C+IE+8">Last 2 versions, Safari 6, IE 8</a></li> On 2016/11/29 17:51:08, saroyanm wrote: > What about linking to -> https://adblockplus.org/requirements > "last 2 versions" doesn't include dev builds of browsers. Acknowledged. I think we should define our own requirements. It would be a huge waste of resources to ask QA to test against the last 3 dev versions, Yandex, SeaMonkey, and Thunderbird. Regarding "last 2 versions, Safari 6, IE 8": - We may want to support more than ABP (EG: Android browser?) - We could match ABP requirements better How about: "> 1%, last 2 Firefox versions, last 2 Chrome versions, last 2 Opera versions, last 2 FirefoxAndroid versions, Safari >= 6, IE >= 6" http://browserl.ist/?q=%3E+1%25%2C+last+2+Firefox+versions%2C+last+2+Chrome+v... Could you weigh in here @Philip?
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): https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:41: <li><a href="http://browserl.ist/?q=last+2+versions%2C+Safari+6%2C+IE+8">Last 2 versions, Safari 6, IE 8</a></li> > "> 1%, last 2 Firefox versions, last 2 Chrome versions, last 2 Opera versions, > last 2 FirefoxAndroid versions, Safari >= 6, IE >= 6" > > http://browserl.ist/?q=%3E+1%25%2C+last+2+Firefox+versions%2C+last+2+Chrome+v... I meant IE >= 8. Sorry guys :D.
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, juliandoucette wrote: > On 2016/11/29 17:51:07, saroyanm wrote: > > What is the reason of naming this as a test ? > > Because "test" is more descriptive than "html". These pages are intended for > testing / demonstrating the features of components. Let's go with demo. https://codereview.adblockplus.org/29361647/diff/29364612/scss/_variables.scss File scss/_variables.scss (right): https://codereview.adblockplus.org/29361647/diff/29364612/scss/_variables.scs... scss/_variables.scss:78: $small-space: 16px !default; On 2016/11/29 20:41:21, juliandoucette wrote: > On 2016/11/29 17:51:07, saroyanm wrote: > > Detail: we are only using $small-space currently > > Don't you think it's a good idea to define medium and large anyway to set a > precedent? > > - We will use them in other components > - By setting them we are defining a naming format and a suggestion about how > many sizes should be defined Acknowledged. https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html File test/content.html (right): https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:34: <h2>Requirements</h2> On 2016/11/29 20:41:22, juliandoucette wrote: > On 2016/11/29 17:51:08, saroyanm wrote: > > Detail: It's table of content not requirements. > > This was intended to be a high level list of requirements that also link to > implementations. Acknowledged We're going to remove this heading, rtl, and browser support items from this demo. https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:40: <li><a id="dir" onclick="changeDirection()">Right-to-left</a></li> On 2016/11/29 20:41:21, juliandoucette wrote: > On 2016/11/29 17:51:07, saroyanm wrote: > > On 2016/11/27 21:45:40, juliandoucette wrote: > > > Note. > > > > > > onclick is officially supported in HTML5. > > > > > > (I didn't know.) > > > > Not really sure what you mean, Event attributes are discouraged to be used: > > > https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Event_attributes#Exam... > > Acknowledged. > > I meant that I think this is a good use case for event attributes, which are now > standard in HTML5. > > My choice was: > > 1. Use event attributes, getElementById, and innerHTML > > OR > > 2. Polyfill addEventListener, DOMContentLoaded, querySelectorAll, and innerText > (via ie8.js) > > -- to support IE 8. > > I guess we will have to include ie8.js later for other components anyway. So I > will change this. Acknowledged. This was made irrelevant because of the previous comment (About table of contents). https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:41: <li><a href="http://browserl.ist/?q=last+2+versions%2C+Safari+6%2C+IE+8">Last 2 versions, Safari 6, IE 8</a></li> On 2016/11/29 20:46:41, juliandoucette wrote: > > > "> 1%, last 2 Firefox versions, last 2 Chrome versions, last 2 Opera versions, > > last 2 FirefoxAndroid versions, Safari >= 6, IE >= 6" > > > > > http://browserl.ist/?q=%3E+1%25%2C+last+2+Firefox+versions%2C+last+2+Chrome+v... > > I meant IE >= 8. Sorry guys :D. Acknowledged. We will discuss this separately. https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:104: <script src="js/demo.js"></script> On 2016/11/27 21:45:38, juliandoucette wrote: > Note. > > Created test/js/demo.js for switching the direction via the link in > requirements. Acknowledged. This will be removed.
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 wrote: > On 2016/11/29 20:41:21, juliandoucette wrote: > > On 2016/11/29 17:51:07, saroyanm wrote: > > > What is the reason of naming this as a test ? > > > > Because "test" is more descriptive than "html". These pages are intended for > > testing / demonstrating the features of components. > > Let's go with demo. Done. https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html File test/content.html (right): https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:34: <h2>Requirements</h2> On 2016/11/30 18:26:01, juliandoucette wrote: > On 2016/11/29 20:41:22, juliandoucette wrote: > > On 2016/11/29 17:51:08, saroyanm wrote: > > > Detail: It's table of content not requirements. > > > > This was intended to be a high level list of requirements that also link to > > implementations. > > Acknowledged > > We're going to remove this heading, rtl, and browser support items from this > demo. Done. https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:40: <li><a id="dir" onclick="changeDirection()">Right-to-left</a></li> On 2016/11/30 18:26:02, juliandoucette wrote: > On 2016/11/29 20:41:21, juliandoucette wrote: > > On 2016/11/29 17:51:07, saroyanm wrote: > > > On 2016/11/27 21:45:40, juliandoucette wrote: > > > > Note. > > > > > > > > onclick is officially supported in HTML5. > > > > > > > > (I didn't know.) > > > > > > Not really sure what you mean, Event attributes are discouraged to be used: > > > > > > https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Event_attributes#Exam... > > > > Acknowledged. > > > > I meant that I think this is a good use case for event attributes, which are > now > > standard in HTML5. > > > > My choice was: > > > > 1. Use event attributes, getElementById, and innerHTML > > > > OR > > > > 2. Polyfill addEventListener, DOMContentLoaded, querySelectorAll, and > innerText > > (via ie8.js) > > > > -- to support IE 8. > > > > I guess we will have to include ie8.js later for other components anyway. So I > > will change this. > > Acknowledged. > > This was made irrelevant because of the previous comment (About table of > contents). Done. https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:41: <li><a href="http://browserl.ist/?q=last+2+versions%2C+Safari+6%2C+IE+8">Last 2 versions, Safari 6, IE 8</a></li> On 2016/11/30 18:26:01, juliandoucette wrote: > On 2016/11/29 20:46:41, juliandoucette wrote: > > > > > "> 1%, last 2 Firefox versions, last 2 Chrome versions, last 2 Opera > versions, > > > last 2 FirefoxAndroid versions, Safari >= 6, IE >= 6" > > > > > > > > > http://browserl.ist/?q=%3E+1%25%2C+last+2+Firefox+versions%2C+last+2+Chrome+v... > > > > I meant IE >= 8. Sorry guys :D. > > Acknowledged. > > We will discuss this separately. Done. https://codereview.adblockplus.org/29361647/diff/29364612/test/content.html#n... test/content.html:104: <script src="js/demo.js"></script> On 2016/11/30 18:26:02, juliandoucette wrote: > On 2016/11/27 21:45:38, juliandoucette wrote: > > Note. > > > > Created test/js/demo.js for switching the direction via the link in > > requirements. > > Acknowledged. > > This will be removed. Done.
I think we need to use a patches in between in case there is a name change, or use hg rename (because it's impossible to see the diffs through the codereview UI). Note: I'll compare it locally no need to upload new patch in this case.
On 2016/12/01 18:31:01, saroyanm wrote: > I think we need to use a patches in between in case there is a name change, or > use hg rename (because it's impossible to see the diffs through the codereview > UI). > > Note: I'll compare it locally no need to upload new patch in this case. (We can't use hg mv because we are adding the files not modifying them.)
On 2016/12/01 18:41:22, juliandoucette wrote: > On 2016/12/01 18:31:01, saroyanm wrote: > > I think we need to use a patches in between in case there is a name change, or > > use hg rename (because it's impossible to see the diffs through the codereview > > UI). > > > > Note: I'll compare it locally no need to upload new patch in this case. > > (We can't use hg mv because we are adding the files not modifying them.) You are right, in that case patch in between should do the magic I guess. Anyway everything looks good LGTM
Note: I think this review can be closed 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
