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

Feat/arbitrary src params #834

Merged
merged 3 commits into from
Nov 28, 2023

Conversation

cjpillsbury
Copy link
Contributor

@cjpillsbury cjpillsbury commented Nov 27, 2023

Mux Player extraSourceParams (prop) / extra-source-params (attr) allows users to specify any arbitrary search/query params for the resultant src URL (based on, e.g. playbackId and other specific props/attrs used to construct the URL).

@cjpillsbury cjpillsbury requested a review from a team as a code owner November 27, 2023 22:09
Copy link

vercel bot commented Nov 27, 2023

@cjpillsbury is attempting to deploy a commit to the Mux Team on Vercel.

A member of the Team first needs to authorize it.

// @ts-ignore
if (!this.hasAttribute(PlayerAttributes.EXTRA_PLAYLIST_PARAMS)) return DEFAULT_EXTRA_PLAYLIST_PARAMS;
return [
...new URLSearchParams(this.getAttribute(PlayerAttributes.EXTRA_PLAYLIST_PARAMS) as string).entries(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: Uses URLSearchParams constructor to more easily/reliably parse the attribute string, which acts as the "source of truth" for the value.

Copy link

codecov bot commented Nov 27, 2023

Codecov Report

Merging #834 (a5a2de3) into main (f7200e7) will increase coverage by 0.04%.
The diff coverage is 96.77%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #834      +/-   ##
==========================================
+ Coverage   82.50%   82.54%   +0.04%     
==========================================
  Files          40       40              
  Lines        8305     8332      +27     
  Branches      457      461       +4     
==========================================
+ Hits         6852     6878      +26     
- Misses       1447     1448       +1     
  Partials        6        6              
Files Coverage Δ
packages/playback-core/src/index.ts 75.63% <100.00%> (ø)
packages/mux-player/src/index.ts 73.57% <96.42%> (+0.39%) ⬆️

Copy link

vercel bot commented Nov 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
elements-demo-create-react-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 28, 2023 2:37pm
elements-demo-nextjs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 28, 2023 2:37pm
elements-demo-svelte-kit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 28, 2023 2:37pm
elements-demo-vanilla ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 28, 2023 2:37pm
elements-demo-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 28, 2023 2:37pm

...state,
// NOTE: since the attribute value is used as the "source of truth" for the property getter,
// moving this below the `...state` spread so it resolves to the default value when unset (CJP)
extraPlaylistParams: el.extraPlaylistParams,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should consider this convention for all of the acrobatics we've had with our generic state update from attr values in attributeChangedCallback(). tl;dr - if our mux-player prop value is always derived from the corresponding mux-player attr value, we should move those below our state spread. (cc @luwes)

Copy link
Contributor

@luwes luwes left a comment

Choose a reason for hiding this comment

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

👍

@cjpillsbury cjpillsbury merged commit a5ad6ed into muxinc:main Nov 28, 2023
13 checks passed
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.

2 participants