Editorial: align with modern IDL prose conventions#167
Merged
Conversation
domenic
reviewed
Apr 22, 2021
Member
domenic
left a comment
There was a problem hiding this comment.
Found a few things. Partially it's a question of how far you want to go in fixing.
notifications.bs
Outdated
| <p>The <dfn constructor for=Notification><code>Notification(title, options)</code></dfn> | ||
| constructor, when invoked, must run these steps: | ||
| <p>The | ||
| <dfn constructor for=Notification lt="Notification(title, options)"><code>new Notification(title, options)</code></dfn> |
notifications.bs
Outdated
|
|
||
| <p>The static <dfn attribute for=Notification><code>maxActions</code></dfn> | ||
| attribute's getter must return the <a>maximum number of actions</a> supported. | ||
| <p>The static <dfn attribute for=Notification><code>maxActions</code></dfn> geter steps are to |
| <a>notification</a>'s <a for=notification>badge URL</a>, <a lt="url serializer">serialized</a>, and | ||
| the empty string if there is no <a>notification</a>'s <a for=notification>badge URL</a> otherwise. | ||
| <ol> | ||
| <li><p>If there is no <a>this</a>'s <a>notification</a>'s <a for=notification>icon URL</a>, then |
Member
There was a problem hiding this comment.
Perhaps for a followup, but it'd be better if these were nullable.
notifications.bs
Outdated
|
|
||
| <p>The <dfn attribute for=Notification><code>requireInteraction</code></dfn> getter steps are to | ||
| return <a>this</a>'s <a>notification</a>'s | ||
| <a for=notification>require interaction preference flag</a>. |
Member
There was a problem hiding this comment.
Probably these should be updated to be booleans? Right now it looks like you're returning a flag which isn't super well-defined (although of course any reader will know what you mean).
notifications.bs
Outdated
|
|
||
| <li><p>Set <var>action</var>'s {{NotificationAction/action}} to | ||
| <var>entry</var>'s <a for=action>name</a>. | ||
| <li><p>Set <var>action</var>'s {{NotificationAction/action}} to <var>entry</var>'s |
Member
There was a problem hiding this comment.
You can use ordered map syntax here.
notifications.bs
Outdated
| </ol> | ||
|
|
||
| <li><p><a>Create a frozen array</a> from <var>frozenActions</var>. | ||
| <li><p><a>Create a frozen array</a> from <var>frozenActions</var>. |
Member
There was a problem hiding this comment.
You forgot to return the result of "create a frozen array". (Preexisting problem.)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Preview | Diff