-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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
correct document picture-in-picture example #33293
Conversation
files/en-us/web/api/document_picture-in-picture_api/using/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/api/document_picture-in-picture_api/using/index.md
Outdated
Show resolved
Hide resolved
Preview URLs External URLs (1)URL:
(comment last updated: 2024-05-08 16:48:07) |
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.
Thanks for the fix, @joshfarrant !
I don't have a definitive review yet because I want a collective answer from the MDN maintainers on #33293 (review).
But one thing to say is that this guide is intended to mirror the working demo at https://mdn.github.io/dom-examples/document-picture-in-picture/ whose source is at https://github.com/mdn/dom-examples/blob/main/document-picture-in-picture/main.js and it's somewhat important that they should not diverge.
You can see that the dom-examples version is correct: https://github.com/mdn/dom-examples/blob/21320eaacc5f902906f3dc8e3d8d7a75fc1a59aa/document-picture-in-picture/main.js#L19-L23
// Returns null if no pip window is currently open
if (!window.documentPictureInPicture.window) {
// Open a Picture-in-Picture window.
...so we need to:
- either update the MDN page to match the GitHub example
- or update the dom-examples version with what's in this PR
I think I favour (2), because I like the early-return pattern (I don't see any guidance either way at https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Writing_style_guide/Code_style_guide/JavaScript). But I'm happy to take care of the GitHub update after this PR is merged.
However I also noticed that the link in this page to the dom-examples source on GitHub is wrong:
> **Note:** You can see the featured demo in action at [Document Picture-in-Picture API Example](https://mdn.github.io/dom-examples/document-picture-in-picture/) (see the full [source code](https://github.com/chrisdavidmills/dom-examples/tree/main/document-picture-in-picture) also).
https://github.com/chrisdavidmills/dom-examples/tree/main/document-picture-in-picture
needs to be:
https://github.com/mdn/dom-examples/tree/main/document-picture-in-picture
I would love it if this PR could include that change.
files/en-us/web/api/document_picture-in-picture_api/using/index.md
Outdated
Show resolved
Hide resolved
@wbamberg Thanks for the response and detailed comments 💖 I've updated the incorrect link in 05b67eb and I've opened a PR on the dom-examples repo here mdn/dom-examples#274 Let me know if I've missed anything 🙂 |
mdn/dom-examples#274 has been merged |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: skyclouds2001 <[email protected]>
Co-authored-by: skyclouds2001 <[email protected]>
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.
Thank you for the fixes, @joshfarrant !
Description
Correct mistake in Document Picture-in-Picture API example and improve clarity of docs.
Motivation
I copy/pasted the current example and it didn't work, so I've tweaked the example a bit to both fix it, and hopefully make it clearer.
Additional details
N/A
Related issues and pull requests