Skip to content

types: Flesh out HTMLMediaElement types#4705

Merged
rschristian merged 1 commit intomainfrom
types/media-element
Feb 24, 2025
Merged

types: Flesh out HTMLMediaElement types#4705
rschristian merged 1 commit intomainfrom
types/media-element

Conversation

@rschristian
Copy link
Copy Markdown
Member

Adds some more types & corrects a few for HTMLMediaElement specifically

@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 99.609%. remained the same
when pulling f32cffa on types/media-element
into d7b4787 on main.

Comment on lines -1266 to +1267
controlsList?: Signalish<string | undefined>;
controlslist?: Signalish<'nodownload' | 'nofullscreen' | 'noremoteplayback' | undefined>;
controlsList?: Signalish<'nodownload' | 'nofullscreen' | 'noremoteplayback' | undefined>;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Might be worth altering AllHTMLElements to get all of its properties from IntrinsicElements, eliminating the need to copy from the more per-element interfaces. Will give that a go separately

Comment on lines -1926 to -1927
playsinline?: Signalish<boolean | undefined>;
playsInline?: Signalish<boolean | undefined>;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

These are specific to HTMLVideoElement, they were moved down to that interface.

Copy link
Copy Markdown
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

LGTM

@rschristian rschristian merged commit 46bace7 into main Feb 24, 2025
7 checks passed
@rschristian rschristian deleted the types/media-element branch February 24, 2025 08:56
@JoviDeCroock JoviDeCroock mentioned this pull request Feb 26, 2025
@piotr-cz
Copy link
Copy Markdown
Contributor

piotr-cz commented Apr 7, 2025

The controlslist attribute allows space separated list of keywords, not a single keyword.

So this is perfectly fine:

<video
  controlsList="nodownload nofullscreen noremoteplayback noplaybackrate"
/>

However change introduced here allows adding just a single keyword: 'nodownload' | 'nofullscreen' | 'noremoteplayback'.

error TS2322: Type '"nodownload nofullscreen noremoteplayback noplaybackrate"' is not assignable to type 'Signalish<"nodownload" | "nofullscreen" | "noremoteplayback" | undefined>'.

References:

@rschristian
Copy link
Copy Markdown
Member Author

rschristian commented Apr 7, 2025

Feel free to make a PR switching that back (and possibly another to MDN as the attribute section doesn't seem to mention more than one is allowed at all).

@piotr-cz
Copy link
Copy Markdown
Contributor

piotr-cz commented Apr 8, 2025

PR: #4744

Indeed, the MDN docs for Video > controlslist are misleading, but quite clear on HTMLMediaElement.controlsList

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants