Skip to content
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

Inline URLUtils for a and area elements #219

Merged
merged 1 commit into from
Oct 6, 2015
Merged

Inline URLUtils for a and area elements #219

merged 1 commit into from
Oct 6, 2015

Conversation

annevk
Copy link
Member

@annevk annevk commented Oct 1, 2015

See whatwg/url#62 for the background. This
defines a mixin used by both a and area elements to define their IDL
members previously defined by the URL standard.

It also fixes a small error in the WorkerLocation text.

@annevk
Copy link
Member Author

annevk commented Oct 1, 2015

Note to self: make this consistent about "the element" vs "element" (former is better).

<h4>API for <code>a</code> and <code>area</code> elements</h4>

<pre class="idl">[NoInterfaceObject, Exposed=Window]
interface <dfn>HTMLHyperLinkElement</dfn> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name should not contain HyperLink; it should be Hyperlink. The document uses hyperlink as all one word, not two words.

Maybe name it with a "mixin" suffix, or "utils"? Everything following the HTML___Element pattern is an actual class, not a mixin. So e.g. HTMLHyperlinkUtils or HTMLHyperlinkElementUtils would help clarify IMO.

@domenic
Copy link
Member

domenic commented Oct 1, 2015

Note to self: make this consistent about "the element" vs "element" (former is better).

I would actually prefer "this element" :)

<li><p>If <var>url</var> or <var>url</var>'s <span data-x="concept-url-host">host</span> is null,
or <var>url</var>'s <span>non-relative flag</span> is set, terminate these steps.</p></li>

<li><p><span>Set the username</span>, given <var>url</var> and the given value.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: there is some inconsistency in how you call other specs algorithms. Here it is "given url and the given value", i.e. as positional arguments. Later it is "with url as url and host state as state override," i.e. named arguments. Might be better to stick to named arguments everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use named arguments when it gets ambiguous. Would love to get rid of that though. Hopefully at some point we have a good meta-language for writing algorithms that has all this stuff covered.

@annevk
Copy link
Member Author

annevk commented Oct 2, 2015

All comments should have been addressed. I left them as separate commits for now.

data-noexport="" data-x="concept-hyperlink-url-set">set the url</dfn> algorithm, which sets this
element's <span data-x="concept-hyperlink-url">url</span> to the <span>resulting parsed URL</span>
of <span data-x="resolve a url">resolving</span> this element's <code
data-x="attr-hyperlink-href">href</code> content attribute value relative to element. If <span
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

relative to element -> relative to this element

@domenic
Copy link
Member

domenic commented Oct 5, 2015

LGTM after fixing the ? vs. # confusion.

@domenic domenic assigned annevk and unassigned domenic Oct 5, 2015
See whatwg/url#62 for the background. This
defines a mixin used by both a and area elements to define their IDL
members previously defined by the URL standard.

It also fixes a small error in the WorkerLocation text.
@annevk annevk merged commit bab6591 into master Oct 6, 2015
@annevk annevk deleted the hyperlink branch October 6, 2015 05:02
nathaneliason pushed a commit to nathaneliason/html that referenced this pull request Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants