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

Document missing RTCPeerConnection»createOffer params #12165

Conversation

sideshowbarker
Copy link
Collaborator

Fixes #12164

Also fixes some broken macros and links.

Fixes #12164

Also fixes some broken macros and links.
@sideshowbarker sideshowbarker requested a review from a team as a code owner January 19, 2022 05:46
@sideshowbarker sideshowbarker requested review from wbamberg and removed request for a team January 19, 2022 05:46
@github-actions github-actions bot added the Content:WebAPI Web API docs label Jan 19, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 19, 2022

Preview URLs

Flaws

None! 🎉

External URLs

URL: /en-US/docs/Web/API/RTCPeerConnection/createOffer
Title: RTCPeerConnection.createOffer()
on GitHub

No new external URLs

(this comment was updated 2022-01-28 01:35:14.891033)

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @sideshowbarker !

I don't think we should add these two options, since as far I can tell they're not in the spec any more. Also I don't think we should add that inline HTML: this hasn't been our practice before, and if we adopt it generally we will end up with a messy mix of HTML and Markdown.

The link fixes are fine though :).

@sideshowbarker
Copy link
Collaborator Author

sideshowbarker commented Jan 19, 2022

I don't think we should add these two options, since as far I can tell they're not in the spec any more.

They’re in the spec at https://w3c.github.io/webrtc-pc/#attributes-2 and they are implemented in browsers — though the section they’re in is “Legacy configuration extensions”, which I suppose should mean that we mark them as deprecated (since the spec says, “Developers are encouraged to use the RTCRtpTransceiver API instead”.

Also I don't think we should add that inline HTML: this hasn't been our practice before, and if we adopt it generally we will end up with a messy mix of HTML and Markdown.

That’s pretty disappointing to hear. I would think that having the utility to direct developer readers to the specific point-of-use places in the docs where they can find the information they need would take precedence over our concerns as maintainers about any internal markup details that don’t end up getting exposed to readers in the rendered content.

Do you have another suggestion about what the cross-references should link to in this case?

@sideshowbarker
Copy link
Collaborator Author

Do you have another suggestion about what the cross-references should link to in this case?

Ah, I now see your comment, “It would be fine for the other page to link to #values”. I don’t agree that would be fine. That just makes things easier for us as maintainers at the cost of making things more difficult for our readers.

@wbamberg
Copy link
Collaborator

This was discussed in the Markdown conversion: https://github.com/mdn/content/discussions/6322. and honestly I thought we had consensus for it. My concern really is that if we have a policy where people can add <span> elements whenever they want to link to something, we'll end up with a mishmash of Markdown and HTML, and it'll almost be worse than just HTML.

If we want to make elements of syntax sections linkable perhaps we could auto-generate ID attributes for dt elements, as we already do for headings.

That just makes things easier for us as maintainers at the cost of making things more difficult for our readers.

Yes. There are trade offs we made moving to Markdown and this was one of them.

@sideshowbarker
Copy link
Collaborator Author

If we want to make elements of syntax sections linkable perhaps we could auto-generate ID attributes for dt elements, as we already do for headings.

Agreed. Is there a reason we’re not already doing that? Is there an open Yari issue for it? If not, should I raise one?

@wbamberg
Copy link
Collaborator

If we want to make elements of syntax sections linkable perhaps we could auto-generate ID attributes for dt elements, as we already do for headings.

Agreed. Is there a reason we’re not already doing that? Is there an open Yari issue for it? If not, should I raise one?

I haven't raised one and I doubt anyone else has. I'd be happy for you to. I can't think of a reason not to do it.

@sideshowbarker
Copy link
Collaborator Author

If we want to make elements of syntax sections linkable perhaps we could auto-generate ID attributes for dt elements, as we already do for headings.

Agreed. Is there a reason we’re not already doing that? Is there an open Yari issue for it? If not, should I raise one?

I haven't raised one and I doubt anyone else has. I'd be happy for you to. I can't think of a reason not to do it.

Opened mdn/yari#5190 with a patch.

@sideshowbarker
Copy link
Collaborator Author

@wbamberg About the options, did you see my reply at #12165 (comment) ?

@wbamberg
Copy link
Collaborator

@wbamberg About the options, did you see my reply at #12165 (comment) ?

Sorry, yes. I think it would be OK to mark them as deprecated.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

👍 thanks!

@wbamberg wbamberg merged commit 98d4703 into main Jan 28, 2022
@wbamberg wbamberg deleted the sideshowbarker/rtcpeerconnection/createoffer-missing-params-document branch January 28, 2022 03:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Content:WebAPI Web API docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with "RTCPeerConnection.createOffer()": missing params
2 participants