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

Issue 29510560: Issue 4633 - Default form styles

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 6 days ago by ire
Modified:
1 week, 4 days 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+450 lines, -0 lines) Patch
M README.md View 1 1 chunk +4 lines, -0 lines 0 comments Download
A pages/forms/advanced.html View 1 2 1 chunk +32 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 1 chunk +8 lines, -0 lines 0 comments Download
A scss/forms/_advanced.scss View 1 1 chunk +64 lines, -0 lines 0 comments Download
A scss/forms/_basic.scss View 1 1 chunk +74 lines, -0 lines 0 comments Download
A scss/forms/_buttons.scss View 1 1 chunk +78 lines, -0 lines 0 comments Download
M scss/main.scss View 1 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 8
ire
1 week, 6 days 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 ...
1 week, 6 days 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> ...
1 week, 6 days 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, ...
1 week, 6 days 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: ...
1 week, 5 days 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: ...
1 week, 5 days 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 ...
1 week, 5 days ago (2017-08-10 17:10:07 UTC) #7
ire
1 week, 4 days ago (2017-08-11 15:25:15 UTC) #8
> 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.
Sign in to reply to this message.

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