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

Extract ResponsiveMediaPlayerView from ResponsiveMediaPlayer #285

Closed
benwiley4000 opened this issue Oct 2, 2018 · 0 comments
Closed

Extract ResponsiveMediaPlayerView from ResponsiveMediaPlayer #285

benwiley4000 opened this issue Oct 2, 2018 · 0 comments

Comments

@benwiley4000
Copy link
Owner

benwiley4000 commented Oct 2, 2018

Right now in order to accommodate the use case where one would like to share the playerContext outside the bounds of the ResponsiveMediaPlayer component, we allow ResponsiveMediaPlayer to inherit from an ancestor PlayerContextProvider if it exists. Otherwise it creates its own PlayerContextProvider.

Being able to nest PlayerContextProviders inside one another to share a common media state is not a feature we intend to support, but users may be misled to expect this behavior given how ResponsiveMediaPlayer works.

In order to avoid this we can extract a ResponsiveMediaPlayerView component which uses a PlayerContextConsumer and assumes an ancestor PlayerContextProvider. If a user wants to nest the player inside a hoisted PlayerContextProvider they can use ResponsiveMediaPlayerView instead of ResponsiveMediaPlayer.

ResponsiveMediaPlayer can then look like this:

const {
  getDisplayText,
  controls,
  fullscreenEnabled,
  showVideo,
  renderVideoDisplay,
  ...rest,
} = this.props;
return (
  <PlayerContextProvider {...rest}>
	<ResponsiveMediaPlayer
	  getDisplayText={getDisplayText}
	  controls={controls}
	  fullscreenEnabled={fullscreenEnabled}
	  showVideo={showVideo}
	  renderVideoDisplay={renderVideoDisplay}
	/>
  </PlayerContextProvider>
);

This is a much better API because it avoids the confusing situation of a ResponsiveMediaPlayer component which receives a bunch of PlayerContextProvider props but doesn't use any of them.

Related: we can remove the word Responsive from each component and probably no one will be upset.

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

No branches or pull requests

1 participant