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

docs: update starter template #5570

Merged
merged 2 commits into from
Nov 14, 2018
Merged

docs: update starter template #5570

merged 2 commits into from
Nov 14, 2018

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 9, 2018

Use a codepen template for the starter template that includes a 7.x
player, video-js element, and a JavaScript created player.

The template is https://codepen.io/gkatsev/pen/GwZegv?editors=1010#0

Fixes #5562

Use a codepen template for the starter template that includes a 7.x
player, video-js element, and a JavaScript created player.

The template is https://codepen.io/gkatsev/pen/GwZegv?editors=1010#0

Fixes #5562
@brandonocasey
Copy link
Contributor

is there any way that we can use unpkg or something for this? That way we don't have to remember to update the version.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 9, 2018

I could update the template.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 9, 2018

Updated

@gkatsev
Copy link
Member Author

gkatsev commented Nov 9, 2018

Also, with this template, it means I can update it without the url changing

@OwenEdwards
Copy link
Member

OwenEdwards commented Nov 9, 2018

My concern with this example is that it actually might be too advanced for some of our users! Having an example using plain HTML and JavaScript, like in the "Getting Started" example, might mean that some users are more likely to create a simple example of an issue they report.

EDIT: It's also not obvious (to me, and therefore I assume to some other less experienced users) where, in the Codepen, you include the video.js files. I dug around and found them, but the old JS Bin put them right there in the HTML editor.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 9, 2018

Ah, yeah, makes sense, let me move it out of settings then.

@brandonocasey
Copy link
Contributor

mostly looks good, I would change the second empty object that is passed to videojs() to a variable and name it options. I think that would make it a bit easier to understand. It may also be a good idea to use var instead of const

@gkatsev
Copy link
Member Author

gkatsev commented Nov 13, 2018

I updated the template to use data-setup again and not using any javascript. I moved the script and styles back into the html. I then updated the links to hide the javascript panel by default.

@gkatsev gkatsev added the needs: LGTM Needs one or more additional approvals label Nov 13, 2018
@brandonocasey
Copy link
Contributor

I think it would make sense to still get a reference to the player in the js panel, even if we hide the panel. Other than that LGTM

@OwenEdwards
Copy link
Member

I agree that there ought to be some JS that references the player, again, like in the https://github.com/videojs/video.js#quick-start.

Also, you're still using a <video-js> element instead of an HTML <video> element - as I mentioned, I think that might be confusing or intimidating to some less experienced users, which might make them less likely to create a reduced test case (unless I'm confused, and this isn't intended as the template for a reduced test case?)

@gkatsev
Copy link
Member Author

gkatsev commented Nov 14, 2018

@OwenEdwards I updated it to switch to <video>.

Also, just realized that codepen has a collection feature, so, we could make a bunch of collections of various other templates, like one with <video-js> and one that's programmatically created, one that uses hooks etc. However, this shouldn't block this PR.

@misteroneill
Copy link
Member

Would it make sense to be more consistent with self-closing tags (e.g. link vs meta) and attributes (some unquoted, some double-quoted, some single-quoted)?

Looks fine overall.

@misteroneill misteroneill removed the needs: LGTM Needs one or more additional approvals label Nov 14, 2018
@gkatsev gkatsev merged commit 287b267 into master Nov 14, 2018
@gkatsev gkatsev deleted the starter-template branch November 14, 2018 18:45
@gkatsev
Copy link
Member Author

gkatsev commented Nov 14, 2018

Merged. I can update the codepen as/if necessary.

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