-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Editorial: Refine link-element section readability #2354
Conversation
615719e
to
fd8c7b2
Compare
@sideshowbarker sorry, can you rebase this now that #2356 is merged? |
fd8c7b2
to
1caf6ab
Compare
Yup, just now rebased and pushed |
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 is great; upon reviewing the two versions side by side, I am convinced this is a huge improvement. It does not depart from existing structure in the ways I feared.
I left some specific comments here. Also on overall structure, I think maybe the sections should go:
- Processing the media attribute
- Processing the type attribute
- Obtaining a resource from a link element
- Processing `Link` headers
- Providing users with a means to follow hyperlinks created using the link element
(see individual comments for what happened to the LinkStyle and Activation behavior sections)
source
Outdated
<dd><code data-x="attr-link-hreflang">hreflang</code></dd> | ||
<dd><code data-x="attr-link-media">media</code></dd> |
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.
If we're rearranging, I wonder if we'd want to group all the type-specific ones (as, scope, sizes, usecache, workertype) together at the bottom and by type. I think that could work especially well in the prose below where we define each attribute, maybe with a <hr>
separating the generic ones from the rel-specific ones. Alternately, see my idea below of moving the attribute definitions for rel-specific attributes into the rel sections.
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.
Yeah, OK, looking at this again, I do think the optimal order for this particular list at least is going to be:
- href, crossorigin, rel, media, nonce, integrity, hreflang, type, referrerpolicy (either in that order---the original order---or alphabetized)
- sizes
- as
- scope, usecache, workertype
- color
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.
And I think that'd be good for the definitions of each attribute too. They don't have to follow the same order as this list (especially the first set), but they should be grouped together and the groups should be in that order, with <hr>
s separating them.
source
Outdated
<p>The semantics of the protocol used (e.g. HTTP) must be followed when fetching external | ||
resources. (For example, redirects will be followed and 404 responses will cause the external | ||
resource to not be applied.)</p> | ||
<p>The <code data-x="attr-link-sizes">sizes</code> attribute is used with 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.
I guess we should be consistent and put the <dfn>
s up in this section, instead of down in the rel sections, for rel-specific attributes? Right now all but sizes
have their <dfn>
up here, it looks like. I'd also be OK with moving all the rel-specific attributes to their appropriate rel sections.
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.
<p>The <code data-x="attr-link-sizes">sizes</code> attribute is used with the <code
I guess we should be consistent and put the
<dfn>
s up in this section, instead of down in the rel sections, for rel-specific attributes?
Yes
Right now all but
sizes
have their<dfn>
up here, it looks like.
Yeah, thanks for catching that.
source
Outdated
|
||
<hr> | ||
<div w-nodev> |
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.
You can keep the <hr>
inside the nodev
section too.
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.
<hr> <div w-nodev>
You can keep the
<hr>
inside thenodev
section too.
OK, moved
source
Outdated
|
||
<li><p>If the <code data-x="attr-link-href">href</code> attribute's value is the empty string, | ||
then abort these steps.</p></li> | ||
<p id="default-media">The default, if the <code data-x="attr-link-media">media</code> attribute is |
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.
I think this goes in the processing model section, probably?
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.
<p id="default-media">The default, if the <code data-x="attr-link-media">media</code> attribute is
I think this goes in the processing model section, probably?
Yes, moved
source
Outdated
<p>The IDL attribute <dfn><code data-x="dom-link-referrerPolicy">referrerPolicy</code></dfn> must | ||
<span>reflect</span> the <code data-x="attr-link-referrerpolicy">referrerpolicy</code> | ||
content attribute, <span>limited to only known values</span>.</p> | ||
<h5>Providing users with a means to follow hyperlinks created using the <code>link</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.
I think the section on activation behavior for link elements that create hyperlinks can be merged into this section (probably at the bottom).
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.
I think the section on activation behavior for link elements that create hyperlinks can be merged into this section (probably at the bottom).
Yup, made it so
source
Outdated
</div> | ||
|
||
<h5>The <code>LinkStyle</code> interface</h5> |
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.
I don't think these examples are related to LinkStyle; they are instead just the usual "examples go at the bottom of an element's section".
What I would do is completely remove this sentence about LinkStyle; it's redundant with the normative IDL. Then we could move these examples into the section on Link type "alternate" and Link type "stylesheet". Maybe we can even add a sentence (probably in the intro, before any subsections) advising people to check out #linkTypes for examples of using the link element.
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.
<h5>The <code>LinkStyle</code> interface</h5>
I don't think these examples are related to LinkStyle
Thanks yeah you’re right of course. Thanks for catching that.
What I would do is completely remove this sentence about LinkStyle; it's redundant with the normative IDL.
Yup, removed
Then we could move these examples into the section on Link type "alternate" and Link type "stylesheet".
Yes—so moved
1caf6ab
to
b1646c3
Compare
(Just rebased against master today; still need to make changes in response to review comments) |
The section of the spec on the `link` element is long and a bit complicated. This editorial change does some reorganization to make it more usable/readable both for implementors and authors. It restructures it into the following parts: 1. Intro and info for required attributes: `href`, `rel`, and `itemprop` 2. `media`, `type`, `hreflang`, `title` (content attributes from HTML4 & before) 3. `as`, `crossorigin`, etc. (content attributes added to `link` after HTML4) 4. IDL attributes and which content attributes they correspond to 5. UA requirements: Processing the `media` attribute 6. UA requirements: Processing the `type` attribute 7. UA requirements: Activation behavior of link elements that create hyperlinks 8. UA requirements: Obtaining a resource from a link element 9. UA requirements: Providing users with a means to follow link hyperlinks 10. UA requirements: Processing `Link` headers 11. The LinkStyle interface Before this change the UA requirements for implementors were intermingled among the document-conformance/informational parts for authors (1, 2, 3 above), and not called out with headings of any kind — which made the section less usable/readable both for implementors and authors. This change doesn’t add or alter any normative requirements; it just moves a few paragraphs around, adds some headings, and alphabetically re-orders some things.
b1646c3
to
65b584d
Compare
* Drop redundant section about LinkStyle (WebIDL already covers it) * Move examples to point of use (and where other examples already are)
OK, changed to that order |
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.
Looking good. I did re-confirm my ordering thoughts.
source
Outdated
<dd><code data-x="attr-link-hreflang">hreflang</code></dd> | ||
<dd><code data-x="attr-link-media">media</code></dd> |
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.
Yeah, OK, looking at this again, I do think the optimal order for this particular list at least is going to be:
- href, crossorigin, rel, media, nonce, integrity, hreflang, type, referrerpolicy (either in that order---the original order---or alphabetized)
- sizes
- as
- scope, usecache, workertype
- color
source
Outdated
<dd><code data-x="attr-link-hreflang">hreflang</code></dd> | ||
<dd><code data-x="attr-link-media">media</code></dd> |
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.
And I think that'd be good for the definitions of each attribute too. They don't have to follow the same order as this list (especially the first set), but they should be grouped together and the groups should be in that order, with <hr>
s separating them.
OK, re-ordered the attributes per #2354 (comment) and #2354 (comment) Take a look and if I overlooked anything lemme know. |
Hyperlink some naked occurrences of the dfn’ed term “external resource link”. And as long as we’re in here, might as well re-wrap a couple of long lines.
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 LGTM; all my comments have been addressed (except one more minor ordering thing I noted). I might do another pass tomorrow but if I find anything I'm sure it'll be minor enough to just push commits myself.
@zcorpan, do you want to take a look too?
source
Outdated
<dd><code data-x="attr-link-referrerpolicy">referrerpolicy</code></dd> | ||
<dd><code data-x="attr-link-as">as</code></dd> | ||
<dd><code data-x="attr-link-scope">scope</code></dd> |
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.
Put scope with usecache/workertype?
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.
Put scope with usecache/workertype?
oofs—yeah, thanks for catching that; fixed
For consistency with hyperlinking added in the previous commit for the term “external resource link”, also add some hyperlinking in a few places for the term “hyperlink”. Plus, hyperlink a couple more references to “external resource link”.
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.
Did not review carefully but it seems this is in a good state to merge.
All pre-existing issues before this PR, but let's get them in while we're here
…852) Fix #822 * Reflow the links section to match sideshowbarker's proposed changes from whatwg/html#2354 * Fix 'browser.include' file that mysteriously got flipped to ANSI encoding...
The section of the spec on the
link
element is long and a bit complicated.This editorial change does some reorganization to make it more usable/readable
both for implementors and authors. It restructures it into the following parts:
href
,rel
, anditemprop
media
,type
,hreflang
,title
(content attributes from HTML4 & before)as
,crossorigin
, etc. (content attributes added tolink
after HTML4)media
attributetype
attributeLink
headersBefore this change the UA requirements for implementors were intermingled among
the document-conformance/informational parts for authors (1, 2, 3 above), and
not called out with headings of any kind — which made the section less
usable/readable both for implementors and authors.
This change doesn’t add or alter any normative requirements; it just moves a few
paragraphs around, adds some headings, and alphabetically re-orders some things.