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

Add HTMLMediaElement.addTextTrack() #36100

Merged
merged 13 commits into from
Sep 30, 2024
Merged

Add HTMLMediaElement.addTextTrack() #36100

merged 13 commits into from
Sep 30, 2024

Conversation

gusborg88
Copy link
Contributor

Description

I made a new page for HTMLMediaElement.addTextTrack(), I could't find what exceptions it throws though so that might need to be fixed.

Motivation

I'm working with this method right now, and I noticed MDN didn't have a page, so I made one.

Additional details

I found most of the info here: https://html.spec.whatwg.org/multipage/media.html#dom-media-addtexttrack

Related issues and pull requests

@gusborg88 gusborg88 requested a review from a team as a code owner September 29, 2024 02:06
@gusborg88 gusborg88 requested review from Elchi3 and removed request for a team September 29, 2024 02:06
@github-actions github-actions bot added Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed labels Sep 29, 2024
whoopsie daisies
Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Hi @gusborg88! Thanks for your contribution. I have a few suggestions to make it follow MDN's latest guidelines and conventions.

Copy link
Contributor Author

@gusborg88 gusborg88 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 edits, looks pretty good! I agree that the usage notes should be moved to the intro, im gonna do that in a minute

Copy link
Contributor

github-actions bot commented Sep 29, 2024

Preview URLs

(comment last updated: 2024-09-30 20:49:33)

@Josh-Cena
Copy link
Member

@gusborg88 Did you push your changes?

@gusborg88
Copy link
Contributor Author

It wont let me edit and keep the edits you made, so I'm just gonna wait until the MDN guy approves this PR with your changes, then make another PR. Or just leave it as is, the things you added fixed most issues anyway

@Josh-Cena
Copy link
Member

I'm not sure what you mean😅

It wont let me edit and keep the edits you made

For the ones I made suggestions, you can click "add suggestion to batch" and then commit all of them. For the ones I verbally suggested changes, you need to edit the file itself. Right now you just clicked "resolve conversation" without actually applying my suggestion. I didn't make edits; I just made suggestions.

so I'm just gonna wait until the MDN guy approves this PR with your changes

Well I'm reviewing the PR, so once you apply my suggestions I will approve it.

@gusborg88
Copy link
Contributor Author

goddammit
Give me a second

@gusborg88
Copy link
Contributor Author

Should be good to merge now

Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for your contribution!

Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Actually one error and one confusion.

Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Great. Thank you for your prompt responses!

@Josh-Cena Josh-Cena changed the title made an addTextTrack() page Add HTMLMediaElement.addTextTrack() Sep 30, 2024
@Josh-Cena Josh-Cena merged commit 935c368 into mdn:main Sep 30, 2024
8 checks passed
pepelsbey pushed a commit to pepelsbey/mdn-content that referenced this pull request Oct 1, 2024
* made an addTextTrack() page

* Forgot a right brace

whoopsie daisies

* Moved usage notes to intro

* i did not add that I what the hell

* the add to batch button wasnt working so im doing this

Co-authored-by: Joshua Chen <[email protected]>

* the add to batch button wasnt working so im doing this

Co-authored-by: Joshua Chen <[email protected]>

* the add to batch button wasnt working so im doing this

Co-authored-by: Joshua Chen <[email protected]>

* the add to batch button wasnt working so im doing this

Co-authored-by: Joshua Chen <[email protected]>

* Update files/en-us/web/api/htmlmediaelement/addtexttrack/index.md

* Apply suggestions from code review

Co-authored-by: Joshua Chen <[email protected]>

* Fixed problem in example

---------

Co-authored-by: Joshua Chen <[email protected]>
fiji-flo pushed a commit that referenced this pull request Oct 2, 2024
* made an addTextTrack() page

* Forgot a right brace

whoopsie daisies

* Moved usage notes to intro

* i did not add that I what the hell

* the add to batch button wasnt working so im doing this

Co-authored-by: Joshua Chen <[email protected]>

* the add to batch button wasnt working so im doing this

Co-authored-by: Joshua Chen <[email protected]>

* the add to batch button wasnt working so im doing this

Co-authored-by: Joshua Chen <[email protected]>

* the add to batch button wasnt working so im doing this

Co-authored-by: Joshua Chen <[email protected]>

* Update files/en-us/web/api/htmlmediaelement/addtexttrack/index.md

* Apply suggestions from code review

Co-authored-by: Joshua Chen <[email protected]>

* Fixed problem in example

---------

Co-authored-by: Joshua Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants