-
Notifications
You must be signed in to change notification settings - Fork 544
Conversation
make the introductory pizza form code blocks easier to read by following proposed element nesting guidelines. consistently continue to use `div` elements between examples and update the examples that were using `p` elements. In an effort to keep the code blocks from getting too long, a radio button option (medium pizza) and a topping option (extra cheese) were deleted from the examples. Prose that referred to these form options were also modified.
update formattting of google and bing search form examples. change the bing input type=“submit” to a button type=“submit” to subtly point out that either element is acceptable as a submit element.
swap out `table` markup with a trimmed down pattern . utilize the similar values used in earlier examples for “customer name” inputs. use a different ID value to indicate that `name` and `id` attributes don’t need to be the same, while also serving to not have name=“name” and id=“name”, thus having 3 “name”s in the example.
update formatting
change example container from `p` to `div` change code example from being within a `code` element to a `xmp` example.
change `p`s to `div`s update the example to utilize `label`s with `for` attributes. add a `type=“password”` attribute to the “password” text input.
update formatting of table `th` elements change `readonly=“readonly”` to just `readonly` change `required=“required”` to just `required` wrap the two end “add” and “save” buttons in a single `div` rather than separage `p` elements.
keep a label as part of the example. don’t want to make authors think it’s ‘good practice’ to not include a consistent accessible name/label for a `select` element.
the original example used `span type=“toolbar”` which is invalid, and an `a` element with an onclick event that would likely be better displayed as a `button`. formatted examle, updated the above mentioned elements, wrapped the code in a `section` and created a new image to reflect the changes in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts. Nothing critical.
<div> | ||
<label> | ||
Preferred delivery time: | ||
<input type="time" min="11:00" max="21:00" step="900"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does the value "900" come from?
I assume it's 15 minutes represented in seconds - but that's not made clear in the explanatory text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the introductory text does indicate that the interval between times is in seconds, but I agree its easy to pass over.
I will make some changes to the prose here to make that more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sections/semantics-forms.include
Outdated
expecting the following parameters sent in an HTTP POST body: | ||
|
||
For the purposes of this introduction, we will assume that the script at | ||
<code>https://pizza.example.com/order.cgi</code> is configured to accept submissions using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a .cgi
file?
(Yes, I'm old enough to know! Worth just changing it to /order
to be less distracting?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a few (12) references to other .cgi files in a other form example's action
attributes.
maybe we change to .php? people should still remember that extension ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edent I took out the .cgi reference in this intro section. but .cgi remains in other examples. You think it's safe to simply remove .cgi from each? Or should we use .php, or another file extension in their place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scottaohara I refer you to https://www.w3.org/Provider/Style/URI
Specifically under "What to leave out"
File name extension. This is a very common one. "cgi", even ".html" is something which will change. You may not be using HTML for that page in 20 years time, but you might want today's links to it to still be valid. The canonical way of making links to the W3C site doesn't use the extension.(how?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @edent :) updated
sections/semantics-forms.include
Outdated
</datalist> | ||
</pre> | ||
|
||
...with the following style sheet applied: | ||
|
||
<pre highlight="css"> | ||
input { height: 75px; width: 49px; background: #D5CCBB; color: black; } | ||
input[type="range"] { height: 75px; width: 49px; background: #D5CCBB; color: #000; } | ||
</pre> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not <xmp>
on the CSS examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call. didn't think to change it since it didn't have an HTML example in it. updated.
sections/semantics-forms.include
Outdated
<label for="multiselect">Select the songs from that you would like on your Act II Mix Tape:</label> | ||
<label for="multiselect"> | ||
Select the songs from that you would like on your Act II Mix Tape: | ||
</label> | ||
<select multiple required id="multiselect" name="act2"> | ||
<option value="s1">It Sucks to Be Me (Reprize)</option> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo. "Repize" should be "Reprise". Even in American English https://www.merriam-webster.com/dictionary/reprise
Provide more context as to where the attribute values are coming from in the `input type=“time”` section of the forms introduction. e.g. `step=“900”` is equivalent to 15 minutes. etc.
remove .cgi and .dll from form examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of things to check please, and a request (you can ignore it if you want) to use lists where things look more like lists than unrelated divs.
It would be nice to add an autocapitalize example to the introduction (which we should have done in adding the attribute in the first place)
sections/semantics-forms.include
Outdated
<label> | ||
<input type="radio" name="size"> Large | ||
</label> | ||
</div> | ||
</fieldset> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a lot like a list of options to me, and I would far prefer it to use a list element. (Same goes for a lot of the examples below).
sections/semantics-forms.include
Outdated
|
||
<xmp highlight="html"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
putting labels in one column of a table and form controls in another is a classic use case for the for
attribute. I think it would still be useful to highlight this case with a simple example.
|
||
<p> | ||
<img src="images/sample-meter.png" width="424" height="270" alt="With the meter elements rendered as inline green bars of varying lengths." /> | ||
</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check that the image is licensed for republication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added note to the acknowledgements and made changes per other comments. Let me know if there's anything else you'd like addressed, @chaals. Thank again :)
change the markup of the introductory example where sizes and toppings are listed to use `ul` & `li` elements instead of `div`s. This commit also adds missing `required` attribute to the part of the introduction that mentioned adding the attribute to the pizza sizes radio buttons, as well as some minor prose source formatting.
add image source attribution [per comment](#1293 (comment))
[per note](#1293 (comment)) adding in a separate example showcasing using `label for=“…”` within the context of a `table`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment. Feel free to fix then merge it.
sections/acknowledgements.include
Outdated
@@ -1157,6 +1157,9 @@ | |||
<a href="http://www.reusableart.com/drop_o.html">by Jessie Wilcox Smith</a> and is in the Public | |||
Domain. | |||
|
|||
<a href="./images/sample-meter.png">The screen shot</a> used within the <{meter}> element | |||
was created <a href="https://github.com/scottaohara">by Scott O'Hara</a>. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the license on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated. will wait for checks to complete and then merge.
This pull request fixes #1290 and largely consists of formatting examples throughout the forms section to be more inline with the WIP styleguide, primarily focusing on indenting for better legibility.
Other notable updates are converting
<p>
wrappers to<div>
s to be consistent between the different examples, and to follow the text in the introduction that states: "Each area within a form is typically represented using a<div>
element."I have modified some examples to remove form controls from from their parent
label
s, and instead addedid
s andfor
attributes to create more examples showcasing this method of labeling form controls.While I was in the file, I also made some minor updates to the prose source formatting and made edits where an update to an example required the prose to be changed.