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

Update embedder code to handle attributes better #3086

Closed
bendk opened this Issue Jan 12, 2018 · 10 comments

Comments

Projects
None yet
2 participants
@bendk
Member

bendk commented Jan 12, 2018

This is the code followup to #3068

  • Remove this item from the Get Embed Code dialog: Scale down size depending on container or display (responsive): data-resizable="true"
  • Remove references to the data-height
  • Made it so data-width also controls the max width of the loading div.
  • Added some extra text on the embedder-offsite page to explain how to make it work on dev instances

To test:

  • Make sure the embedder still works
  • Check that the loading image is the same width as the embedder, using the embedder-offsite page
  • Double check that the Get Embed Code dialog is updated

@bendk bendk added this to the Sprint 30 milestone Jan 12, 2018

@bendk bendk self-assigned this Jan 12, 2018

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Jan 31, 2018

  • Get Embed Code on old video / language pages has text: "If you would like the widget to be responsive, you can add data-resizable="true"." Is this as expected?

screenshot from 2018-01-31 02 10 44

  • Video in the embedder on new-style video page in team context does not expand to full-screen mode properly:

screenshot from 2018-01-31 02 14 02

  • Get Embed Code on new-style video page does not populate data-width parameter. Instead, it includes data-width="px":

screenshot from 2018-01-31 02 13 46

  • Onsite embedders on the language pages play the original language of the video rather than the current subtitle language.

  • MP4 videos do not resize great if the browser width is smaller than the value of data-width:

screenshot from 2018-01-31 03 00 52

  • /embedder-offsite page does not have the explanation of parameters in the embed code and their use. Instead, it has the list of video URLs used on the page.
@bendk

This comment has been minimized.

Member

bendk commented Jan 31, 2018

I'm pretty sure the MP4 issue was the same issue that caused fullscreen mode to fail. I think it's fixed now that I merged the code from dev into the branch.

@bendk

This comment has been minimized.

Member

bendk commented Jan 31, 2018

For the embedder-offsite page, I thought listing the parameters was redundant considering we have other help pages. My understanding is that page is more for debugging, not for general use. Tell me if I'm wrong about that one though.

bendk added a commit that referenced this issue Jan 31, 2018

@bendk

This comment has been minimized.

Member

bendk commented Jan 31, 2018

In general, I tried to remove all reference to the data-width parameter. As far as I can tell it's not needed.

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Feb 1, 2018

  • The embedder on the subtitle diffing page does not load. The JS console displays the following error:

embedder.js:12422 Uncaught ReferenceError: videoData is not defined
at G.d.setFromListingResponse (embedder.js:12422)
at Object.success (embedder.js:12400)
at l (embedder.js:39)
at Object.fireWith [as resolveWith] (embedder.js:39)
at T (embedder.js:39)
at XMLHttpRequest.r (embedder.js:39)

  • New team landing page also has embedders not loading with a similar JS error.

  • I don't quite understand the part about removing data-width. I think the option to explicitly set the size of the embedder on external pages is very useful for making nice looking web pages. Also, the demo (/embedder-offsite/) page does use data-width in the examples.

Lastly, regarding the /embedder-offsite/ page, I think it is ok not to duplicate the help text from the wiki, but maybe better remove "To test, make sure you have the following videos added to your dev instance" text before merging this code?

@bendk

This comment has been minimized.

Member

bendk commented Feb 1, 2018

The change for data-width is to make it optional. You can use it to explicitly set the width, and that's what we test on embedder-offsite. If you don't set it, then the embedder gets the width of its container, which seems simpler in most cases.

Can you explain why you want that text removed? Whenever I use that page to test, I always forget to add the videos, which is why I put it in. As far as I know, it's only used for testing, so why not add the note?

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Feb 2, 2018

https://github.com/pculture/unisubs/wiki/Embed-Code-Usage-Guide lists /embedder-offsite/ as a demo page for end-users, so having test instructions on the demo page does not look very neat.

Instead, why not add the videos used on that page to the script that automatically populates a new instance with data? The script could also upload some subtitles for convenience of testing.

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Feb 2, 2018

If I paste the embed code from a modal to a Web page as is, without specifying the data-width, the embedder expands to the whole width of the browser (typically entire screen). This looks horrible and will likely cause lots of questions from inexperienced users. I think it would be better to leave data-width in the modals, setting it to some reasonable number - for example, the same size as we use on the video page.

@bendk

This comment has been minimized.

Member

bendk commented Feb 2, 2018

Good point on doing this with a script, I made #3108 for that and removed the instructions from the page.

I'm still not convinced that giving data-width is a good idea, since I think the more likely use case is pasting that code into a website that you already have. In that case, you don't want data-width set, since you want the embedder to fill the width of whatever element you put it inside. If we give data-width, and it's smaller than the width of the parent element, then it will look weird.

bendk added a commit that referenced this issue Feb 2, 2018

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Feb 7, 2018

Deployed.

@PCF-Testing PCF-Testing closed this Feb 7, 2018

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