Issue 4633 - Default form styles

Issue 4633 - Default form styles

[ire, 2017-08-09 09:21:23]

I decided to start as small as possible, just with a reset/normalize. Actual
styling of the elements will come in a following patch.

[juliandoucette, 2017-08-09 13:23:58]

Thanks Ire!

Here are my first impressions.

https://codereview.adblockplus.org/29510560/diff/29510561/pages/forms.html
File pages/forms.html (right):

https://codereview.adblockplus.org/29510560/diff/29510561/pages/forms.html#ne...
pages/forms.html:36: <h2>Advanced text</h2>
Perhaps we should separate (at least):

- basic form fields
- advanced form fields
- buttons

Because:

- many of our pages want button styles but not forms
- most of our forms are not advanced

If we separate them then we can exclude unnecessary styles from our stylesheets.

https://codereview.adblockplus.org/29510560/diff/29510561/scss/_forms.scss
File scss/_forms.scss (right):

https://codereview.adblockplus.org/29510560/diff/29510561/scss/_forms.scss#ne...
scss/_forms.scss:1:
/*******************************************************************************
Missing license header.

Note: I'm not sure that we need license headers in these files based on Kai's
feedback.

https://codereview.adblockplus.org/29510560/diff/29510561/scss/_forms.scss#ne...
scss/_forms.scss:7: /* 1. Reset
Where did you get these reset styles from?

https://codereview.adblockplus.org/29510560/diff/29510561/scss/_forms.scss#ne...
scss/_forms.scss:138: [hidden]
I think this applies to non-forms too no?

[ire, 2017-08-09 17:04:04]

Answered your questions below

https://codereview.adblockplus.org/29510560/diff/29510561/pages/forms.html
File pages/forms.html (right):

https://codereview.adblockplus.org/29510560/diff/29510561/pages/forms.html#ne...
pages/forms.html:36: <h2>Advanced text</h2>
On 2017/08/09 13:23:57, juliandoucette wrote:
> Perhaps we should separate (at least):
> 
> - basic form fields
> - advanced form fields
> - buttons
> 
> Because:
> 
> - many of our pages want button styles but not forms
> - most of our forms are not advanced
> 
> If we separate them then we can exclude unnecessary styles from our
stylesheets.

Do you mean separate the styles into separate SCSS files? Because they are
already separate here in the markup.

https://codereview.adblockplus.org/29510560/diff/29510561/scss/_forms.scss
File scss/_forms.scss (right):

https://codereview.adblockplus.org/29510560/diff/29510561/scss/_forms.scss#ne...
scss/_forms.scss:1:
/*******************************************************************************
On 2017/08/09 13:23:57, juliandoucette wrote:
> Missing license header.
> 
> Note: I'm not sure that we need license headers in these files based on Kai's
> feedback.

Acknowledged. Will add it in the next patch.

https://codereview.adblockplus.org/29510560/diff/29510561/scss/_forms.scss#ne...
scss/_forms.scss:7: /* 1. Reset
On 2017/08/09 13:23:57, juliandoucette wrote:
> Where did you get these reset styles from?

A combination of:

- Normalize.css
- Your last codereview
- My own research

https://codereview.adblockplus.org/29510560/diff/29510561/scss/_forms.scss#ne...
scss/_forms.scss:138: [hidden]
On 2017/08/09 13:23:57, juliandoucette wrote:
> I think this applies to non-forms too no?

Yes you're right. I guess they may be more commonly used with forms, but can be
applied to every element. I will remove them from here and we can add it to a
different file in a separate commit.

[juliandoucette, 2017-08-09 20:00:11]

Quick followup.

https://codereview.adblockplus.org/29510560/diff/29510561/pages/forms.html
File pages/forms.html (right):

https://codereview.adblockplus.org/29510560/diff/29510561/pages/forms.html#ne...
pages/forms.html:36: <h2>Advanced text</h2>
On 2017/08/09 17:04:03, ire wrote:
> Do you mean separate the styles into separate SCSS files? Because they are
> already separate here in the markup.

Yes. +Separate demo pages. Sorry for the confusion.

https://codereview.adblockplus.org/29510560/diff/29510561/scss/_forms.scss
File scss/_forms.scss (right):

https://codereview.adblockplus.org/29510560/diff/29510561/scss/_forms.scss#ne...
scss/_forms.scss:7: /* 1. Reset
On 2017/08/09 17:04:03, ire wrote:
> On 2017/08/09 13:23:57, juliandoucette wrote:
> > Where did you get these reset styles from?
> 
> A combination of:
> 
> - Normalize.css
> - Your last codereview
> - My own research

Acknowledged.

Thanks!

Note: We should probably credit normalize.css somewhere.

[ire, 2017-08-10 11:46:40]

> Note: We should probably credit normalize.css somewhere.

Done.

https://codereview.adblockplus.org/29510560/diff/29510561/pages/forms.html
File pages/forms.html (right):

https://codereview.adblockplus.org/29510560/diff/29510561/pages/forms.html#ne...
pages/forms.html:36: <h2>Advanced text</h2>
On 2017/08/09 20:00:11, juliandoucette wrote:
> On 2017/08/09 17:04:03, ire wrote:
> > Do you mean separate the styles into separate SCSS files? Because they are
> > already separate here in the markup.
> 
> Yes. +Separate demo pages. Sorry for the confusion.

Thanks. Done.

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/basic.html
File pages/forms/basic.html (right):

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/basic.h...
pages/forms/basic.html:14: <li><a href="#radios">Radios</a></li>
I'm undecided whether all these qualify as "basic form fields". What do you
think?

[juliandoucette, 2017-08-10 17:10:07]

Thanks Ire!

It will take me a little while longer to review this SCSS.

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/advance...
File pages/forms/advanced.html (right):

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/advance...
pages/forms/advanced.html:16: <h2>Advanced text</h2>
NIT: We indent by 2 spaces.

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/basic.html
File pages/forms/basic.html (right):

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/basic.h...
pages/forms/basic.html:4: <p>Default styles for native form elements.</p>
Note: I think this paragraph is technically a subtitle and should be placed
inside a header with the heading above. Perhaps we should fix this across the
board on our demo pages as a separate noissue.

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/basic.h...
pages/forms/basic.html:6: <p><a href="forms">Back to Forms</a></p>
NIT: We don't have back buttons on the other demo pages. I think we should add
them via breadcrumbs or header navigation in a separate issue/review.

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/basic.h...
pages/forms/basic.html:14: <li><a href="#radios">Radios</a></li>
On 2017/08/10 11:46:40, ire wrote:
> I'm undecided whether all these qualify as "basic form fields". What do you
> think?

I think they do. Are there any in particular you are unsure about?

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/index.html
File pages/forms/index.html (right):

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/index.h...
pages/forms/index.html:6: <nav>
NIT: It's kindof annoying to navigate two levels deep. I suggest we move these
to the index page.

https://codereview.adblockplus.org/29510560/diff/29511599/scss/forms/_buttons...
File scss/forms/_buttons.scss (right):

https://codereview.adblockplus.org/29510560/diff/29511599/scss/forms/_buttons...
scss/forms/_buttons.scss:28: button
Note: I think we may want to share visual (not reset) styles between link
buttons and form buttons. I'm saying this so that you have it in mind later.

[ire, 2017-08-11 15:25:15]

> It will take me a little while longer to review this SCSS.

No worries. I've addressed your other comments in the meantime

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/advance...
File pages/forms/advanced.html (right):

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/advance...
pages/forms/advanced.html:16: <h2>Advanced text</h2>
On 2017/08/10 17:10:06, juliandoucette wrote:
> NIT: We indent by 2 spaces.

Done.

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/basic.html
File pages/forms/basic.html (right):

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/basic.h...
pages/forms/basic.html:4: <p>Default styles for native form elements.</p>
On 2017/08/10 17:10:07, juliandoucette wrote:
> Note: I think this paragraph is technically a subtitle and should be placed
> inside a header with the heading above. 

Yes.

> Perhaps we should fix this across the board on our demo pages as a separate
noissue.

Yes.

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/basic.h...
pages/forms/basic.html:6: <p><a href="forms">Back to Forms</a></p>
On 2017/08/10 17:10:06, juliandoucette wrote:
> NIT: We don't have back buttons on the other demo pages. I think we should add
> them via breadcrumbs or header navigation in a separate issue/review.

Ack. Done.

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/basic.h...
pages/forms/basic.html:14: <li><a href="#radios">Radios</a></li>
On 2017/08/10 17:10:07, juliandoucette wrote:
> On 2017/08/10 11:46:40, ire wrote:
> > I'm undecided whether all these qualify as "basic form fields". What do you
> > think?
> 
> I think they do. Are there any in particular you are unsure about?

Okay. I was thinking, Selects, Radios, Checkboxes. Not that they are "advanced"
fields per se, but I can imagine forms that only require basic text and
textareas. But that might be going to nuanced.

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/index.html
File pages/forms/index.html (right):

https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/index.h...
pages/forms/index.html:6: <nav>
On 2017/08/10 17:10:07, juliandoucette wrote:
> NIT: It's kindof annoying to navigate two levels deep. I suggest we move these
> to the index page.

Done.

https://codereview.adblockplus.org/29510560/diff/29511599/scss/forms/_buttons...
File scss/forms/_buttons.scss (right):

https://codereview.adblockplus.org/29510560/diff/29511599/scss/forms/_buttons...
scss/forms/_buttons.scss:28: button
On 2017/08/10 17:10:07, juliandoucette wrote:
> Note: I think we may want to share visual (not reset) styles between link
> buttons and form buttons. I'm saying this so that you have it in mind later.

Okay, noted.

[juliandoucette, 2017-08-24 12:35:24]

This seems mostly good. My only complaints were that we could move a few
selectors and document better. But it's unclear how adding a CSS reset to
website-defaults will effect this. I think that some of these styles will be
made irrelevant as a result. Therefore I think we should add a CSS reset to
website-defaults first and then update this Patchset on top of that.

https://codereview.adblockplus.org/29510560/diff/29512690/README.md
File README.md (right):

https://codereview.adblockplus.org/29510560/diff/29512690/README.md#newcode7
README.md:7: - normalize.css: [MIT
License](https://github.com/necolas/normalize.css/blob/master/LICENSE.md) by
Nicolas Gallagher and Jonathan Neal
Shouldn't we mention that our CSS contains part of normalize.css (or
equivalent)? This reads like we actually included normalize.css as part of our
code.

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_advance...
File scss/forms/_advanced.scss (right):

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_advance...
scss/forms/_advanced.scss:28: [type="number"]::-webkit-inner-spin-button,
NIT: I think we should mention that we are fixing Chrome
[*](https://github.com/necolas/normalize.css/blob/master/normalize.css#L361)

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_advance...
scss/forms/_advanced.scss:39: [type="week"]
NIT: These styles seem to differ from normalize.css. I think we should document
why.

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_advance...
scss/forms/_advanced.scss:48: [type="search"]
NIT: We could add [type="search"] to the previous selector

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_advance...
scss/forms/_advanced.scss:55: [type="search"]::-webkit-search-decoration
NIT: We could add these to the previous selector too.

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_advance...
scss/forms/_advanced.scss:60: ::-webkit-file-upload-button
NIT: I Think we should point out what we are doing here
[*](https://github.com/necolas/normalize.css/blob/master/normalize.css#L389)

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_basic.scss
File scss/forms/_basic.scss (right):

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_basic.s...
scss/forms/_basic.scss:34: margin: 0px;
NIT: We should mention that this effects Firefox and Safari
[*](https://github.com/necolas/normalize.css/blob/master/normalize.css#L235) and
that the rest of these styles seem to be opinionated.

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_basic.s...
scss/forms/_basic.scss:56: fieldset,
NIT: We should mention that the rest of these styles are opinionated. We should
also test
[this](https://github.com/necolas/normalize.css/commit/cbdd0b1683304ab9132e572dc0a53f927f56d833)
in-case we should display legend as table.

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_basic.s...
scss/forms/_basic.scss:61: padding: 0px;
NIT: We should mention that this is correcting Firefox
- [*](https://github.com/necolas/normalize.css/blob/master/normalize.css#L311)
- [*](https://github.com/necolas/normalize.css/blob/master/normalize.css#L318)

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_basic.s...
scss/forms/_basic.scss:67: overflow: auto;
NIT: I think we should mention that this is removing the default scrollbar in IE
[*](https://github.com/necolas/normalize.css/blob/master/normalize.css#L342)

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_basic.s...
scss/forms/_basic.scss:73: padding: 0px;
NIT: I think we should mention that we are removing the padding in IE 10-
[*](https://github.com/necolas/normalize.css/blob/master/normalize.css#L351)

[juliandoucette, 2017-08-24 12:37:17]

Additional notes:

- I didn't look for missing styles that should be included in any depth, I only
reviewed styles that were included
- I think we should meet about how we want these input fields to look by default
later because some of these styles may be redundant later (e.g. If we set no
padding now and then set padding elsewhere later, etc)

[ire, 2017-09-13 07:59:58]

Hey! I have:

1. Rebased on the latest website-defaults (Patch Set 4)
2. Addressed your comments (Patch Set 5)

https://codereview.adblockplus.org/29510560/diff/29512690/README.md
File README.md (right):

https://codereview.adblockplus.org/29510560/diff/29512690/README.md#newcode7
README.md:7: - normalize.css: [MIT
License](https://github.com/necolas/normalize.css/blob/master/LICENSE.md) by
Nicolas Gallagher and Jonathan Neal
On 2017/08/24 12:35:23, juliandoucette wrote:
> Shouldn't we mention that our CSS contains part of normalize.css (or
> equivalent)? This reads like we actually included normalize.css as part of our
> code.

Done.

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_advance...
File scss/forms/_advanced.scss (right):

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_advance...
scss/forms/_advanced.scss:28: [type="number"]::-webkit-inner-spin-button,
On 2017/08/24 12:35:23, juliandoucette wrote:
> NIT: I think we should mention that we are fixing Chrome
> [*](https://github.com/necolas/normalize.css/blob/master/normalize.css#L361)

Done.

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_advance...
scss/forms/_advanced.scss:39: [type="week"]
On 2017/08/24 12:35:23, juliandoucette wrote:
> NIT: These styles seem to differ from normalize.css. I think we should
document
> why.

Done.

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_advance...
scss/forms/_advanced.scss:48: [type="search"]
On 2017/08/24 12:35:23, juliandoucette wrote:
> NIT: We could add [type="search"] to the previous selector

Done.

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_advance...
scss/forms/_advanced.scss:55: [type="search"]::-webkit-search-decoration
On 2017/08/24 12:35:23, juliandoucette wrote:
> NIT: We could add these to the previous selector too.

Done.

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_advance...
scss/forms/_advanced.scss:60: ::-webkit-file-upload-button
On 2017/08/24 12:35:23, juliandoucette wrote:
> NIT: I Think we should point out what we are doing here
> [*](https://github.com/necolas/normalize.css/blob/master/normalize.css#L389)

Done.

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_basic.scss
File scss/forms/_basic.scss (right):

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_basic.s...
scss/forms/_basic.scss:34: margin: 0px;
On 2017/08/24 12:35:24, juliandoucette wrote:
> NIT: We should mention that this effects Firefox and Safari
> [*](https://github.com/necolas/normalize.css/blob/master/normalize.css#L235)
and
> that the rest of these styles seem to be opinionated.

Done.

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_basic.s...
scss/forms/_basic.scss:61: padding: 0px;
On 2017/08/24 12:35:24, juliandoucette wrote:
> NIT: We should mention that this is correcting Firefox
> - [*](https://github.com/necolas/normalize.css/blob/master/normalize.css#L311)
> - [*](https://github.com/necolas/normalize.css/blob/master/normalize.css#L318)

No longer relevant with update

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_basic.s...
scss/forms/_basic.scss:67: overflow: auto;
On 2017/08/24 12:35:24, juliandoucette wrote:
> NIT: I think we should mention that this is removing the default scrollbar in
IE
> [*](https://github.com/necolas/normalize.css/blob/master/normalize.css#L342)

Done.

https://codereview.adblockplus.org/29510560/diff/29512690/scss/forms/_basic.s...
scss/forms/_basic.scss:73: padding: 0px;
On 2017/08/24 12:35:23, juliandoucette wrote:
> NIT: I think we should mention that we are removing the padding in IE 10-
> [*](https://github.com/necolas/normalize.css/blob/master/normalize.css#L351)

Done.

[juliandoucette, 2017-09-18 13:13:22]

LGTM + NITs

https://codereview.adblockplus.org/29510560/diff/29543555/pages/forms/advance...
File pages/forms/advanced.html (right):

https://codereview.adblockplus.org/29510560/diff/29543555/pages/forms/advance...
pages/forms/advanced.html:26: <section id="advanced-text">
NIT: I think that we should exclude the file input until we use it (which we may
never).

https://codereview.adblockplus.org/29510560/diff/29543555/pages/forms/basic.html
File pages/forms/basic.html (right):

https://codereview.adblockplus.org/29510560/diff/29543555/pages/forms/basic.h...
pages/forms/basic.html:23: <label for="invalid-text">Invalid text</label>
NIT: I would exclude the invalid state from this commit because we haven't
styled it.

https://codereview.adblockplus.org/29510560/diff/29543555/scss/forms/_advance...
File scss/forms/_advanced.scss (right):

https://codereview.adblockplus.org/29510560/diff/29543555/scss/forms/_advance...
scss/forms/_advanced.scss:60: ::-webkit-file-upload-button
NIT: See comment in advanced.html

[ire, 2017-09-19 08:06:49]

My original intention was originally to continue working with in this review,
but perhaps it makes sense to do the more cosmetic styling in a separate commit,
so I will create another issue for that.

https://codereview.adblockplus.org/29510560/diff/29543555/pages/forms/advance...
File pages/forms/advanced.html (right):

https://codereview.adblockplus.org/29510560/diff/29543555/pages/forms/advance...
pages/forms/advanced.html:26: <section id="advanced-text">
On 2017/09/18 13:13:21, juliandoucette wrote:
> NIT: I think that we should exclude the file input until we use it (which we
may
> never).

Makes sense. Done.

https://codereview.adblockplus.org/29510560/diff/29543555/pages/forms/basic.html
File pages/forms/basic.html (right):

https://codereview.adblockplus.org/29510560/diff/29543555/pages/forms/basic.h...
pages/forms/basic.html:23: <label for="invalid-text">Invalid text</label>
On 2017/09/18 13:13:21, juliandoucette wrote:
> NIT: I would exclude the invalid state from this commit because we haven't
> styled it.

I think I would rather keep it, since we will style it the next patch/commit.

https://codereview.adblockplus.org/29510560/diff/29543555/scss/forms/_advance...
File scss/forms/_advanced.scss (right):

https://codereview.adblockplus.org/29510560/diff/29543555/scss/forms/_advance...
scss/forms/_advanced.scss:60: ::-webkit-file-upload-button
On 2017/09/18 13:13:22, juliandoucette wrote:
> NIT: See comment in advanced.html

Done.