editorial: attr/method intros are notes (closes #379) #438
Conversation
marcoscaceres
force-pushed
the
intro-notes
branch
2 times, most recently
from
5d26278
to
fde1d35
Compare
index.html
Outdated
| <a>shippingAddress</a> is populated when the user provides a shipping | ||
| address. It is null by default. When a user provides a shipping | ||
| address, the <a>shipping address changed algorithm</a> runs. | ||
| <p class="note"> |
There was a problem hiding this comment.
Hmm. I am less sure about the attributes than about the methods. Since their behavior isn't defined in these sections at all, maybe it is OK to leave the text as non-note, as long as we don't use any normative keywords. The risk with methods is that people might think the notes are what specs their behavior.
There was a problem hiding this comment.
A totally different style you might consider:
- Get rid of the separate sections for each attribute.
- Add normative text to the overall PaymentRequest interface section, or maybe to an attributes subsection: "The shippingAddress and shippingOption attributes must return the values they are initialized to. The onshippingaddresschange and onshippingoptionchange attributes are event handle IDL attributes for the shippingaddresschange and shippingoptionchange events, respectively."
- Introduce "domintro" style notes for web developers. Examples: https://html.spec.whatwg.org/#pixel-manipulation https://w3c.github.io/IndexedDB/#request-api. (You'd have separate domintro boxes for each method + the constructor, plus one for the attributes, I think.)
I think this would help especially with the attributes, to separate the normative parts from the web developer info (e.g. "default value is null" is for the web developer; the normative algorithm that sets it to null takes place in the constructor).
There was a problem hiding this comment.
Oh, also note that PaymentResponse currently uses a completely different style for its attributes, instead of having a heading per attribute :-/.
There was a problem hiding this comment.
Hmm. I am less sure about the attributes than about the methods.
Agree. Removed "note". Making statements of fact with non-RFC2119 words seems good (even if they mostly benefit developers).
Oh, also note that PaymentResponse currently uses a completely different style for its attributes, instead of having a heading per attribute :-/.
We should switch those to be headings (separate bug) - personally, I find it frustrating when attributes are not in the ToC - one should be able to see/access the API shape there too.
There was a problem hiding this comment.
We should switch those to be headings (separate bug) - personally, I find it frustrating when attributes are not in the ToC - one should be able to see/access the API shape there too.
+1
There was a problem hiding this comment.
@adrianhopebailie I filed: #442
Kindly please also note you can "react" to a comment via github's interface:
It saves generating an email with only "+1" in it, while allowing you to throw support behind a proposal.
| <section data-dfn-for="PaymentRequest" data-link-for="PaymentRequest"> | ||
| <h2> | ||
| <dfn>onshippingaddresschange</dfn> attribute | ||
| </h2> | ||
| <p> |
There was a problem hiding this comment.
This is inconsistent with the rest (missing class="note"). But as per above I'd be OK with omitting class="note" for all the attributes...
There was a problem hiding this comment.
Leaving attributes without notes.
There was a problem hiding this comment.
I'll convert the PaymentResponse attributes into sections/headings to match.
index.html
Outdated
| <p class="note"> | ||
| The <a>total</a>.<a data-lt= | ||
| "PaymentItem.amount">amount</a>.<a data-lt= | ||
| "PaymentCurrencyAmount.value">value</a> can't be a negative number. |
There was a problem hiding this comment.
Maybe "Algorithms in this specification that accept a PaymentDetails dictionary will throw if the total.amount.value is a negative number."
index.html
Outdated
| remaining user interface to be closed). | ||
| </p> | ||
| <div class="note"> | ||
| <div class="note" title="Dealing with blocked browser UI"> |
There was a problem hiding this comment.
Honestly this feels like a normative requirement, not a note.
There was a problem hiding this comment.
Agree. I've changed the second "may" to an RFC2119 "MAY" and un-noted these two paragraphs.
marcoscaceres
force-pushed
the
intro-notes
branch
from
122d07a
to
cbd6cd0
Compare
marcoscaceres
force-pushed
the
intro-notes
branch
from
cbd6cd0
to
7f2e088
Compare
marcoscaceres
force-pushed
the
intro-notes
branch
from
6bfaac1
to
3a18a8d
Compare
index.html
Outdated
| @@ -121,7 +121,7 @@ | |||
| authorizing as necessary across the flow. | |||
| </p> | |||
| <p> | |||
| The details of how to fulfill a payment request for a given <a>payment | |||
| The details of how to fulfil a payment request for a given <a>payment | |||
There was a problem hiding this comment.
The original word was correct here
There was a problem hiding this comment.
Ah right, my editor is misconfigured I think: https://en.wiktionary.org/wiki/fulfil
We should change those to en-us. Or is there a difference between "fulfill" and "fulfil" that I don't know about?
index.html
Outdated
| <h2> | ||
| <dfn>onshippingoptionchange</dfn> attribute | ||
| </h2> | ||
| <p class="note"> |
There was a problem hiding this comment.
This should not be a note, to be consistent
index.html
Outdated
| <p class="note"> | ||
| If the web page wishes to update the payment request then it can | ||
| call <a>updateWith()</a> and provide a promise that will resolve | ||
| with a <a>PaymentDetailsUpdate</a> dictionary containing changed |
There was a problem hiding this comment.
Maybe point out that you don't have to provide a promise. E.g. "provide a PaymentDetailsUpdate dictionary, or a promise for one, containing..."
marcoscaceres
force-pushed
the
intro-notes
branch
from
7218c33
to
217b760
Compare
Preview | Diff