Inline RTCSessionDescriptionInit description#33963
Conversation
…lSessionDescriptionInit Add missing documentation for RTCSessionDescriptionInit, RTCLocalSessionDescriptionInit interfaces. Also correct several links referencing those interfaces. Also add a clear explanation of the relationship between RTCSessionDescription, RTCSessionDescriptionInit, and RTCLocalSessionDescriptionInit interfaces. Fixes mdn#33962
github-actions
Bot
added
Content:WebAPI
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
skyclouds2001
left a comment
skyclouds2001
left a comment
There was a problem hiding this comment.
I think the two dictionaries are better not to placed in separate pages, as they both only contains two properties and both only used in one method
Josh-Cena
left a comment
Josh-Cena
left a comment
There was a problem hiding this comment.
Hi! Unfortunately we don't document dictionaries on MDN, unless they are reused by a lot of methods (and even in such case we would usually just copy the property list around). Could you inline the description in the places where they are used instead?
Combine RTCSessionDescriptionInit and RTCLocalSessionDescriptionInit descriptions within the RTCSessionDescription page. Additionally, update any related links.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Josh-Cena
left a comment
Josh-Cena
left a comment
There was a problem hiding this comment.
We never mention the dictionary's name at all, because it's not a code entity. It should look like this:
- : An object with the following properties:
- `property1`
- : What it does
And then for the errors, you would just say "the sessionDescription parameter is invalid".
|
Do you mean I should update the PR to remove any references to the directory name |
|
Yes, they should be removed. |
|
I'll keep my PR open for now, but I'm not sure if I can make any more changes, especially considering that the distinction of dictionary names is required since they are part of the WebRTC API. I believe it is important to retain the dictionary name in the documentation. If someone wants to take over the PR, I'm happy to close it. |
|
@bc-lee FYI @Josh-Cena is correct - the fact that the spec mentions these dictionaries is irrelevant. Specs are written for browser developers, while MDN interprets the specification for webapp developers. In most cases we've found that having separate dictionaries is unhelpful for web app developers - they have to jump around to find relevant information, and often the compatibility data for the dictionary depends on where it is used. Because of that, the best way to properly track compatibility is to expand the dictionary. We're not dogmatic about it. For some APIs it does actually make sense to define the objects passed to APIs as dictionaries. Generally this is when you have a general API that can take numerous possible objects depending on the context in which it is used. I haven't looked at this closely, but based on the comments we won't be merging this as is. I'm sure it will be useful though if modified to match MDN conventions. |
|
This pull request has merge conflicts that must be resolved before it can be merged. |
|
Two days ago, a pull request relevant to this one (#34051) was merged. I'd like to hear from @Josh-Cena and @hamishwillee on whether you believe the merged PR achieves the same level of consistency as requested in this pull request. |
|
@bc-lee I'm not sure what you want to express? That PR corrects incorrect information to correct information. While it doesn't remove documentation for dictionaries, that's fine because it's (a) making a correctness improvement, which is more important than editorial conventions (b) not adding more documentation pages for dictionaries. Of course it would be better if that PR simply removed the mention to |
|
@bc-lee, do you have time to come back to this? |
|
Yes, but not today or tomorrow (atm I'm busy with other things). I'll come back to this in a few days. |
Josh-Cena
added
the
awaiting response
…x.md Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Josh-Cena
changed the title
Josh-Cena
left a comment
Josh-Cena
left a comment
There was a problem hiding this comment.
I've simply inlined the descriptions in each page and reordered some sections. I did not do other major changes. Thanks for the PR!
* fix: Add missing documentation for RTCSessionDescriptionInit, RTCLocalSessionDescriptionInit Add missing documentation for RTCSessionDescriptionInit, RTCLocalSessionDescriptionInit interfaces. Also correct several links referencing those interfaces. Also add a clear explanation of the relationship between RTCSessionDescription, RTCSessionDescriptionInit, and RTCLocalSessionDescriptionInit interfaces. Fixes #33962 * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Apply Code Review Combine RTCSessionDescriptionInit and RTCLocalSessionDescriptionInit descriptions within the RTCSessionDescription page. Additionally, update any related links. * Update files/en-us/web/api/rtcsessiondescription/index.md Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Update files/en-us/web/api/rtcsessiondescription/rtcsessiondescription/index.md * Update * Update files/en-us/web/api/rtcpeerconnection/setlocaldescription/index.md Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Fix * Fix others --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
4 participants
Description
Add missing explanation of RTCSessionDescriptionInit and RTCLocalSessionDescriptionInit dictionaries. Also correct several links referencing those interfaces.
Motivation
Additional details
Related issues and pull requests
Fixes #33962