Skip to content
This repository has been archived by the owner on Feb 20, 2024. It is now read-only.

Fixed options in Plyr.svelte, so passed in options are used #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MartinElsaesser
Copy link

@MartinElsaesser MartinElsaesser commented Jun 11, 2020

The passed in options to are not used. This is because you are not passing the options object to the plyr class but rather the opts() function which adds properties to the options object. Alternatively you could also replace the formerly used opts with opts(). I just think it is more meaningful to actually pass in the options object.

P.S.: I reworked as little as needed to get it working for me. But I could see this code getting a lot neater by replacing

$: opts();

function opts() {
	if (!options.hasOwnProperty('hideYouTubeDOMError')) {
		options.hideYouTubeDOMError = true;
	}
	return options;
}

with

$: {
	if (!options.hasOwnProperty('hideYouTubeDOMError')) {
		options.hideYouTubeDOMError = true;
	}
}

@benwoodward
Copy link
Owner

Thanks, LGTM. One issue is that you've changed the indentation for 3 files. Are you able to revert the indentation back to 2 space tabs?

@MartinElsaesser
Copy link
Author

I'll try. What about my other proposed fix? Changing opt() to an expression svelte then evaluates?

@MartinElsaesser
Copy link
Author

MartinElsaesser commented Jun 11, 2020

Hi again, so for going from tabs to spaces I would need to change too much of my vs-code config for what it's worth. So here is the edited code as a snippet.

<script>
  import { onMount, onDestroy, createEventDispatcher } from 'svelte';
  import Plyr from 'plyr';
  import '../plyr.scss';

  export let eventsToEmit = [];
  export let options = {};
  export let player = {};
  let plyrDiv;
  const dispatch = createEventDispatcher();

  $: if (!options.hasOwnProperty('hideYouTubeDOMError')) {
    options.hideYouTubeDOMError = true;
  }

  onMount(async () => {
    player = new Plyr(plyrDiv.firstChild, options);
    eventsToEmit.forEach(event => dispatchOnPlayerEvent(event));
  });

  onDestroy(() => {
    try {
      player.destroy();
    } catch (e) {
      if (!(options.hideYouTubeDOMError && e.message === 'The YouTube player is not attached to the DOM.')) {
        // eslint-disable-next-line no-console
        console.error(e);
      }
    }
  });

  function dispatchOnPlayerEvent (event) {
    player.on(event, data => dispatch(event, data.detail.plyr))
  }
</script>

<div bind:this={plyrDiv}>
  <slot></slot>
</div>

It is even neater now.

@benwoodward
Copy link
Owner

I just downloaded VS Code to check this, it's super easy to fix. Just click the indentation button in the status bar, change it to 2 space tabs, save the file, repeat for each affected file, commit the changes, push the commit, done.

image
image

VS Code saves these settings on a project by project basis, so it shouldn't be a problem.

@MartinElsaesser
Copy link
Author

Well it's just that I can't find the Spaces button in the status bar. Nevertheless I implemented the wrapper myself, because I needed some specific extra functionality for use with Routify.

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

Successfully merging this pull request may close these issues.

2 participants