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

Issue 29510560: Issue 4633 - Default form styles (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 1 week ago by ire
Modified:
2 months, 3 weeks ago
Reviewers:
juliandoucette
Base URL:
https://hg.adblockplus.org/website-defaults
Visibility:
Public.

Description

Issue 4633 - Default form styles

Patch Set 1 #

Total comments: 11

Patch Set 2 : Separate forms into basic, advanced, and buttons #

Total comments: 13

Patch Set 3 : Addressed NITs #

Total comments: 21

Patch Set 4 : Rebase on website-defaults with reset and namespaced content #

Total comments: 6

Patch Set 5 : Added more explanatory comments #

Patch Set 6 : Addressed NITs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+431 lines, -1 line) Patch
M README.md View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
A pages/forms/advanced.html View 1 2 3 4 5 1 chunk +23 lines, -0 lines 0 comments Download
A pages/forms/basic.html View 1 2 1 chunk +139 lines, -0 lines 0 comments Download
A pages/forms/buttons.html View 1 2 1 chunk +48 lines, -0 lines 0 comments Download
M pages/index.html View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
A scss/forms/_advanced.scss View 1 2 3 4 5 1 chunk +54 lines, -0 lines 0 comments Download
A scss/forms/_basic.scss View 1 2 3 4 1 chunk +81 lines, -0 lines 0 comments Download
A scss/forms/_buttons.scss View 1 2 3 1 chunk +73 lines, -0 lines 0 comments Download
M scss/main.scss View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 13
ire
4 months, 1 week ago (2017-08-09 09:17:56 UTC) #1
ire
I decided to start as small as possible, just with a reset/normalize. Actual styling of ...
4 months, 1 week ago (2017-08-09 09:21:23 UTC) #2
juliandoucette
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#newcode36 pages/forms.html:36: <h2>Advanced text</h2> ...
4 months, 1 week ago (2017-08-09 13:23:58 UTC) #3
ire
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#newcode36 pages/forms.html:36: <h2>Advanced text</h2> On 2017/08/09 13:23:57, ...
4 months, 1 week ago (2017-08-09 17:04:04 UTC) #4
juliandoucette
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#newcode36 pages/forms.html:36: <h2>Advanced text</h2> On 2017/08/09 17:04:03, ire wrote: ...
4 months ago (2017-08-09 20:00:11 UTC) #5
ire
> 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#newcode36 pages/forms.html:36: ...
4 months ago (2017-08-10 11:46:40 UTC) #6
juliandoucette
Thanks Ire! It will take me a little while longer to review this SCSS. https://codereview.adblockplus.org/29510560/diff/29511599/pages/forms/advanced.html ...
4 months ago (2017-08-10 17:10:07 UTC) #7
ire
> It will take me a little while longer to review this SCSS. No worries. ...
4 months ago (2017-08-11 15:25:15 UTC) #8
juliandoucette
This seems mostly good. My only complaints were that we could move a few selectors ...
3 months, 3 weeks ago (2017-08-24 12:35:24 UTC) #9
juliandoucette
Additional notes: - I didn't look for missing styles that should be included in any ...
3 months, 3 weeks ago (2017-08-24 12:37:17 UTC) #10
ire
Hey! I have: 1. Rebased on the latest website-defaults (Patch Set 4) 2. Addressed your ...
3 months ago (2017-09-13 07:59:58 UTC) #11
juliandoucette
LGTM + NITs https://codereview.adblockplus.org/29510560/diff/29543555/pages/forms/advanced.html File pages/forms/advanced.html (right): https://codereview.adblockplus.org/29510560/diff/29543555/pages/forms/advanced.html#newcode26 pages/forms/advanced.html:26: <section id="advanced-text"> NIT: I think that ...
2 months, 3 weeks ago (2017-09-18 13:13:22 UTC) #12
ire
2 months, 3 weeks ago (2017-09-19 08:06:49 UTC) #13
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.
Sign in to reply to this message.

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