Issue 29365594: Issue 4633 - Default form styles (Closed)
|
Created:
Nov. 29, 2016, 4:29 p.m. by juliandoucette Modified:
July 5, 2017, 9:24 p.m. CC:
ire Visibility:
Public. |
DescriptionWe would like to provide sensible default styles for native form elements.
Patch Set 1 #
Total comments: 76
MessagesTotal messages: 6
On 2016/11/29 16:30:03, juliandoucette wrote: Hey guys, This is the first draft of default styles for native form elements. I haven't done extensive cross browser tests yet, but I would like to ask for your first impressions. I'd also like to ask @Thomas to help out since this may be highly relevant to hub.eyeo.com (I'd like to use these styles in our base). @Thomas Let me know if you are on board and I will help you set up (This issue builds upon #4607 https://codereview.adblockplus.org/29361647/).
First round is ready. Mostly comments that we already covered in previous reviews, but decided to keep as a reminder here as well. Also we need to merge with actual revision and update accrodingly, example: we renamed folder from test to demo. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_button-links.... File scss/_button-links.scss (right): https://codereview.adblockplus.org/29365594/diff/29365595/scss/_button-links.... scss/_button-links.scss:31: line-height: 2em; Detail: I think we decided to use line-heights without units. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_button-links.... scss/_button-links.scss:44: .submit, Weak suggrestion: I think we can move this style to the forms.scss, or maybe whole file. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss File scss/_forms.scss (right): https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:34: * 2. Remove the margin in Firefox and Safari. Detail: Why exactly in Firefox and Safari ? I think we don't know exact list of the browsers that we are removing margins from. Having general comment I think will makes more sense. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:39: optgroup, Why do we apply this styles to the optgroup ? Ex. margins doesn't have any sense for this element I guess. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:43: margin: 0; /* 2 */ Detail: please specify units where possible. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:43: margin: 0; /* 2 */ I don't think that we need to be that specific regarding the comments. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:45: font-size: 100%; /* 1 */ Deatail: I think you want to specify 1em here ? https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:46: line-height: inherit; I assume you want to setup this according to the current element font-size, not according to the parent ? https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:53: input[type="date"], This input types are not supported across most of the browsers. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:64: * 1. Prevent a WebKit bug where (2) destroys native `audio` and `video` Not really sure if I understand how this comment is relevant to the rule ? https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:70: html [type="button"], /* 1 */ [type="button"] selector will be more consistent with others. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:75: -webkit-appearance: button; /* 2 */ I don't think why we need to use this non standard feature. Even if we do, why do we only specify the appearance for webkit browsers ? https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:79: * Remove the inner border and padding in Firefox. I can't see if this have any effect on my side. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:92: * Restore the focus styles unset by the previous rule. Again: why do we need this non standard reset ? Also I can't test this, maybe it has some particular effect to some special FF version ? Or maybe I'm missing smth. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:104: * 2. Remove the padding in IE 10-. I don't think that this style is specific for only IE 10- I assume this might be relevant to other browsers as well. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:114: * 1. Correct the odd appearance in Chrome and Safari. What odd appearance is this fixing, I can't see any result on my side. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:134: outline: 5px auto -webkit-focus-ring-color; I don't think that we want to use a webkit specific color, do we ? https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:141: [hidden] Are we planing to use hidden attribute ? I can't see any place where we are using it currently. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:149: fieldset { Detail: the opening braces should go to the next line. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:165: line-height: inherit; deatail: As far as I remember we decided to specify line-height without units, according to the fint-size. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:182: [type="date"], Some of this input type, like date and time are not supported in most of the browsers, we need to find out which of them are we going to use. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:216: overflow: auto; Why we need to setup overflow to auto ? https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:223: -webkit-appearance: none; The way we are designing select boxes are not reliable and probably will have issues in most of the browsers that are not supporting SVGs or Appearance attribute. We probably can check if specific attribute exists or not before applying some of this styles. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:227: background-image: url("data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' version='1.1' width='32' height='24' viewBox='0 0 32 24'><polygon points='0,0 32,0 16,24' style='fill: rgb%28138, 138, 138%29'></polygon></svg>"); I think we can use PNG and made a wider support. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:228: background-size: 9px 6px; I think we can use background shorthand property. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:231: padding-right: 25px; I thought we awere planing to use EMs instead. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:237: padding-right: 25px; You already setup padding right above. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:246: margin-right: 0.15em; What if the label will be before the checkbox ? Even if we planing to have label on after the checkbox we need to consider RTL. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:252: @extend .button; I'll suggest not to use @extend if there is no real need for it. Currently we can have a general style I guess for some set of input types and buttons, same applies to the style below. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:281: .disabled input, Why not to use disabled attribute ? https://codereview.adblockplus.org/29365594/diff/29365595/test/button-links.html File test/button-links.html (right): https://codereview.adblockplus.org/29365594/diff/29365595/test/button-links.h... test/button-links.html:47: <li><a id="dir" onclick="changeDirection()">Right-to-left</a></li> Detail: we decided to remove Change direction and Browser support from another review, probably we will reconsider them later on. https://codereview.adblockplus.org/29365594/diff/29365595/test/forms.html File test/forms.html (right): https://codereview.adblockplus.org/29365594/diff/29365595/test/forms.html#new... test/forms.html:53: <li><a id="dir" onclick="changeDirection()">Right-to-left</a></li> Detail: we decided to remove Change direction and Browser support from another review, probably we will reconsider them later on.
Thanks Manvel. We probably want to meet about this one soon to clear up some miscommunication about the reset styles and investigate whether or not some of them apply to us. We should probably also sync about how we are using progressive enhancement here (EG: select). The rest was pretty straightforward :) https://codereview.adblockplus.org/29365594/diff/29365595/scss/_button-links.... File scss/_button-links.scss (right): https://codereview.adblockplus.org/29365594/diff/29365595/scss/_button-links.... scss/_button-links.scss:31: line-height: 2em; On 2016/12/06 18:26:53, saroyanm wrote: > Detail: I think we decided to use line-heights without units. Acknowledged. I will change this. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_button-links.... scss/_button-links.scss:44: .submit, On 2016/12/06 18:26:53, saroyanm wrote: > Weak suggrestion: I think we can move this style to the forms.scss, or maybe > whole file. Acknowledged. Some websites may have no forms or forms on only one page. Therefore, I think it's better to separate forms and buttons so that we can optionally include them in separate CSS files (EG: a separate forms.css on a website with only one form). https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss File scss/_forms.scss (right): https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:34: * 2. Remove the margin in Firefox and Safari. On 2016/12/06 18:26:54, saroyanm wrote: > Detail: Why exactly in Firefox and Safari ? I think we don't know exact list of > the browsers that we are removing margins from. > Having general comment I think will makes more sense. Acknowledged. This was taken from normalize. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:39: optgroup, On 2016/12/06 18:26:54, saroyanm wrote: > Why do we apply this styles to the optgroup ? Ex. margins doesn't have any sense > for this element I guess. Acknowledged. Don't know. This was taken from normalize. I guess 1 & 2 apply to optgroup in some way. I can investigate. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:43: margin: 0; /* 2 */ On 2016/12/06 18:26:54, saroyanm wrote: > Detail: please specify units where possible. Acknowledged. Will-do. Again, taken from normalize. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:45: font-size: 100%; /* 1 */ On 2016/12/06 18:26:54, saroyanm wrote: > Deatail: I think you want to specify 1em here ? Acknowledged. Maybe. I'll investigate. I think normalize did this because some browsers set font-size to under 100%. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:46: line-height: inherit; On 2016/12/06 18:26:53, saroyanm wrote: > I assume you want to setup this according to the current element font-size, not > according to the parent ? Inherit is the default value. This is a reset style. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:53: input[type="date"], On 2016/12/06 18:26:54, saroyanm wrote: > This input types are not supported across most of the browsers. Acknowledged. - They are supported by HTML5 - They fallback to text on browsers that do not support them - These styles prevent odd browser specific styling (specifically in webkit browsers) It makes sense for us to use the correct HTML5 inputs in this case because they affect the keyboard layout on mobile devices. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:64: * 1. Prevent a WebKit bug where (2) destroys native `audio` and `video` On 2016/12/06 18:26:53, saroyanm wrote: > Not really sure if I understand how this comment is relevant to the rule ? Apparently applying "-webkit-appearance: button" to "[type="button"]" causes a bug on fullscreen video for webkit based browsers on Android 4. And applying the same style to "html [type="button"]" is a workaround. Again, this was taken from normalize.css. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:70: html [type="button"], /* 1 */ On 2016/12/06 18:26:53, saroyanm wrote: > [type="button"] selector will be more consistent with others. Acknowledged. See comment above. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:75: -webkit-appearance: button; /* 2 */ On 2016/12/06 18:26:53, saroyanm wrote: > I don't think why we need to use this non standard feature. > Even if we do, why do we only specify the appearance for webkit browsers ? Because webkit based mobile browsers apply default styles to buttons that cannot be overwritten without changing this property. EG: Border radius and gradient on submit buttons in Safari for iOS. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:79: * Remove the inner border and padding in Firefox. On 2016/12/06 18:26:54, saroyanm wrote: > I can't see if this have any effect on my side. Acknowledged. Looks like this is not relevant to the latest versions of firefox. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:92: * Restore the focus styles unset by the previous rule. On 2016/12/06 18:26:54, saroyanm wrote: > Again: why do we need this non standard reset ? > Also I can't test this, maybe it has some particular effect to some special FF > version ? Or maybe I'm missing smth. Acknowledged. See comment above. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:104: * 2. Remove the padding in IE 10-. On 2016/12/06 18:26:53, saroyanm wrote: > I don't think that this style is specific for only IE 10- I assume this might be > relevant to other browsers as well. Acknowledged. Again, this comes from normalize. Apparently it is specific to IE10-. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:114: * 1. Correct the odd appearance in Chrome and Safari. On 2016/12/06 18:26:55, saroyanm wrote: > What odd appearance is this fixing, I can't see any result on my side. Search input has border radius by default on chrome and safari. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:134: outline: 5px auto -webkit-focus-ring-color; On 2016/12/06 18:26:54, saroyanm wrote: > I don't think that we want to use a webkit specific color, do we ? Acknowledged. This is restoring the default behaviour on webkit browsers and is ignored by other browsers. We may or may not want to define our own focus styles. We are not currently. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:141: [hidden] On 2016/12/06 18:26:53, saroyanm wrote: > Are we planing to use hidden attribute ? > I can't see any place where we are using it currently. It's common to use this attribute to hide custom form elements EG: replacing a selectbox with a customized dropdown. This also provides a standard way to hide elements. Perhaps we should consider moving it to base.scss? https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:149: fieldset { On 2016/12/06 18:26:54, saroyanm wrote: > Detail: the opening braces should go to the next line. Acknowledged. (Sorry) https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:165: line-height: inherit; On 2016/12/06 18:26:53, saroyanm wrote: > deatail: As far as I remember we decided to specify line-height without units, > according to the fint-size. Acknowledged. The intention here was to unset browser default line-height. We can investigate when/if this applies to our requirements. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:182: [type="date"], On 2016/12/06 18:26:54, saroyanm wrote: > Some of this input type, like date and time are not supported in most of the > browsers, we need to find out which of them are we going to use. Acknowledged. I got this list from the HTML5 spec. Also see comment above. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:216: overflow: auto; On 2016/12/06 18:26:53, saroyanm wrote: > Why we need to setup overflow to auto ? Acknowledged. I don't know :/ . Will investigate. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:223: -webkit-appearance: none; On 2016/12/06 18:26:54, saroyanm wrote: > The way we are designing select boxes are not reliable and probably will have > issues in most of the browsers that are not supporting SVGs or Appearance > attribute. > We probably can check if specific attribute exists or not before applying some > of this styles. I styled select this way to specifically address this problem. - webkit-appearance of none normalizes select on webkit browsers - moz-appearance of none normalizes select on mozilla browsers - svg background normalizes the caret icon on modern browsers and highDPI screens The intention is to make the select box as consistent as possible across browsers. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:227: background-image: url("data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' version='1.1' width='32' height='24' viewBox='0 0 32 24'><polygon points='0,0 32,0 16,24' style='fill: rgb%28138, 138, 138%29'></polygon></svg>"); On 2016/12/06 18:26:54, saroyanm wrote: > I think we can use PNG and made a wider support. SVG is smaller and supports highDPI. We cannot replace the default caret icon on browsers that don't support SVG anyway. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:228: background-size: 9px 6px; On 2016/12/06 18:26:54, saroyanm wrote: > I think we can use background shorthand property. Maybe. This may affect browser support. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:231: padding-right: 25px; On 2016/12/06 18:26:54, saroyanm wrote: > I thought we awere planing to use EMs instead. The padding-right is in px because the caret icon width is in px. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:237: padding-right: 25px; On 2016/12/06 18:26:54, saroyanm wrote: > You already setup padding right above. Acknowledged. Maybe I got this backwards :/ https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:246: margin-right: 0.15em; On 2016/12/06 18:26:53, saroyanm wrote: > What if the label will be before the checkbox ? > Even if we planing to have label on after the checkbox we need to consider RTL. Acknowledged. Good point. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:252: @extend .button; On 2016/12/06 18:26:54, saroyanm wrote: > I'll suggest not to use @extend if there is no real need for it. > Currently we can have a general style I guess for some set of input types and > buttons, same applies to the style below. Acknowledged. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:281: .disabled input, On 2016/12/06 18:26:54, saroyanm wrote: > Why not to use disabled attribute ? Because this class is intended to apply to the paragraph or container containing the label and the input field. (And container element disabled attribute does not apply to child elements consistently in all browsers.) https://codereview.adblockplus.org/29365594/diff/29365595/test/button-links.html File test/button-links.html (right): https://codereview.adblockplus.org/29365594/diff/29365595/test/button-links.h... test/button-links.html:47: <li><a id="dir" onclick="changeDirection()">Right-to-left</a></li> On 2016/12/06 18:26:55, saroyanm wrote: > Detail: we decided to remove Change direction and Browser support from another > review, probably we will reconsider them later on. Acknowledged. I will change this. https://codereview.adblockplus.org/29365594/diff/29365595/test/forms.html File test/forms.html (right): https://codereview.adblockplus.org/29365594/diff/29365595/test/forms.html#new... test/forms.html:53: <li><a id="dir" onclick="changeDirection()">Right-to-left</a></li> On 2016/12/06 18:26:55, saroyanm wrote: > Detail: we decided to remove Change direction and Browser support from another > review, probably we will reconsider them later on. Acknowledged. I will change this.
Notes from review meeting. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_button-links.... File scss/_button-links.scss (right): https://codereview.adblockplus.org/29365594/diff/29365595/scss/_button-links.... scss/_button-links.scss:20: * Button links TODO: - change to button.scss (move native button styles here) - rename .button and .submit to .primary-button .secondary-button - name may change https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss File scss/_forms.scss (right): https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:20: * Forms TODO: - move button styles to button.scss - remove browsers and numeration from comments - generally keep normalize only when useful and not overridden https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:34: * 2. Remove the margin in Firefox and Safari. will-do. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:45: font-size: 100%; /* 1 */ On 2016/12/07 17:31:09, juliandoucette wrote: > On 2016/12/06 18:26:54, saroyanm wrote: > > Deatail: I think you want to specify 1em here ? > > Acknowledged. > > Maybe. I'll investigate. I think normalize did this because some browsers set > font-size to under 100%. Note: Will change this to em instead of % https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:46: line-height: inherit; On 2016/12/07 17:31:09, juliandoucette wrote: > On 2016/12/06 18:26:53, saroyanm wrote: > > I assume you want to setup this according to the current element font-size, > not > > according to the parent ? > > Inherit is the default value. This is a reset style. Acknowledged, will investigate, and determine if we override this. - Consider when parent font-size may be changed https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:53: input[type="date"], On 2016/12/07 17:31:09, juliandoucette wrote: > On 2016/12/06 18:26:54, saroyanm wrote: > > This input types are not supported across most of the browsers. > > Acknowledged. > > - They are supported by HTML5 > - They fallback to text on browsers that do not support them > - These styles prevent odd browser specific styling (specifically in webkit > browsers) > > It makes sense for us to use the correct HTML5 inputs in this case because they > affect the keyboard layout on mobile devices. See comment below. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:59: -webkit-appearance: listbox; - will investigate if -webkit-appearance is necessary - will investigate if/when we should support all these input types (probably not necessary) https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:134: outline: 5px auto -webkit-focus-ring-color; Agreed. Let's use a non-webkit color. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:182: [type="date"], On 2016/12/07 17:31:08, juliandoucette wrote: > On 2016/12/06 18:26:54, saroyanm wrote: > > Some of this input type, like date and time are not supported in most of the > > browsers, we need to find out which of them are we going to use. > > Acknowledged. > > I got this list from the HTML5 spec. > > Also see comment above. See comment above. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:227: background-image: url("data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' version='1.1' width='32' height='24' viewBox='0 0 32 24'><polygon points='0,0 32,0 16,24' style='fill: rgb%28138, 138, 138%29'></polygon></svg>"); - will double check double backgrounds - will double check svg fallback - will consider png fallback https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:237: padding-right: 25px; On 2016/12/07 17:31:08, juliandoucette wrote: > On 2016/12/06 18:26:54, saroyanm wrote: > > You already setup padding right above. > > Acknowledged. > > Maybe I got this backwards :/ Verified. I got this backwards. https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:252: @extend .button; On 2016/12/07 17:31:09, juliandoucette wrote: > On 2016/12/06 18:26:54, saroyanm wrote: > > I'll suggest not to use @extend if there is no real need for it. > > Currently we can have a general style I guess for some set of input types and > > buttons, same applies to the style below. > > Acknowledged. We will move button styles to button.scss https://codereview.adblockplus.org/29365594/diff/29365595/scss/_forms.scss#ne... scss/_forms.scss:281: .disabled input, - Will use attribute here instead
I'm going to break this down into smaller reviews to make it more manageable. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
