-
Notifications
You must be signed in to change notification settings - Fork 22
Factor out maplike interfaces property reads and writes #305
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
Conversation
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
[SecureContext, Exposed=(Window,Worker)] | ||
interface PropertyReadMap { | ||
readonly maplike<DOMString, InteractionOutput>; | ||
}; | ||
|
||
[SecureContext, Exposed=(Window,Worker)] | ||
interface PropertyWriteMap { | ||
readonly maplike<DOMString, InteractionInput>; | ||
}; |
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 WebIDL syntax does not seem very intuitive to me. I would have expected something like
interface PropertyReadMap extends Map<DOMString, InteractionOutput>
.
Anyhow, WebIDL maplikes
are to be described like this in the body.
Even though I raised the issue I am not sure myself whether if it is worth using maplikes ...
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.
Web IDL syntax. https://heycam.github.io/webidl/#idl-maplike
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 the way to express the idea in Web IDL, still better than using object
.
A nice thing about |
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 PR it's fine. Some minor discussion points to be addressed.
index.html
Outdated
@@ -1838,15 +1849,18 @@ <h3> | |||
</section> | |||
|
|||
<section> | |||
<h3>The <dfn>PropertyMap</dfn> type</h3> | |||
<h3>The <dfn>PropertyReadMap</dfn> type</h3> | |||
<p> | |||
Represents a map of <a>Property</a> names as strings to a value that the <a>Property</a> can take. It is used as a property bag for interactions that involve multiple <a>Properties</a> at once. |
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 am not sure that value is the right term here. With the current API model, we return a map of strings and InteractionOutput which is a gateway to the real value. Should we rephrase to something like this:
" Represents a map of Property names as strings to an InteractionOutput that can be used to retrieve/contains the value of Property"?
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.
Makes sense, thanks. Fixed.
<p> | ||
Represents a map of <a>Property</a> names as strings to a value that the <a>Property</a> can take. It is used as a property bag for interactions that involve multiple <a>Properties</a> at once. | ||
</p> | ||
</section> |
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.
See my other comment.
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.
Fixed.
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
Note: IF we merge this PR we should update TS definitions also (can be done in a subsequent path) |
The PR above should covert the issue ✔️ |
Scripting Call 2021-03-01: Syntactic changes seems OK. |
It also did some white space corrections automatically.
Preview | Diff