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

Youtube playlist #947

Closed
wants to merge 15 commits into from
Closed

Conversation

Alex-Jordan
Copy link
Contributor

Starting with this to see if anything about the documentation raises flags for you. I'll continue to commit parts to this branch and you could package it into one commit when it's all done.

There is some magic distinction between YT users and channels that I cannot fully grok. Sometimes I can get a channel embedded, sometimes not. Sometimes a user's uploads, sometimes not. So I am bailing on that kind of extension for now. Playlists only.

@rbeezer
Copy link
Collaborator

rbeezer commented Aug 26, 2018

Looks good. Let's put "playlist" into <term> like it is being defined. Generally, I'm trying not to make the Author's Guide a compendium of one example of everything, but just enough to whet one's appetite, and enough guidance for someone to figure it out. So feel free to make one of each thing for the sample article, and you only need to provide descriptions of options in the Author's Guide, though there should be at least one playlist example there. Maybe a small table of the different attributes with an example specifications and an explanation? Single, list, playlist, channel?

@Alex-Jordan
Copy link
Contributor Author

Alex-Jordan commented Aug 29, 2018

XSLT in place for HTML. Do you want to take a look and comment?

@Alex-Jordan
Copy link
Contributor Author

Figures 16.12 and 16.13 here, if you'd like to peek at output:
http://spot.pcc.edu/~ajordan/temp/section-video.html

@rbeezer
Copy link
Collaborator

rbeezer commented Aug 29, 2018

Looks good. Didn't require too much surgery, no?

Right now, if an author uses a single-video ID and includes a stray space before or after, that one video will get packaged as a 1-video playlist, no? How about a plain normalize-space() before even entering the choose? Then your test for the multiple-ID scenario will be more fool-proof? (And it might mean you don't need the later instance of normalize-space(), which I have not checked carefully.)

@Alex-Jordan
Copy link
Contributor Author

I can do what you are suggesting. It feels

@Alex-Jordan
Copy link
Contributor Author

Oops

@Alex-Jordan Alex-Jordan reopened this Aug 30, 2018
@Alex-Jordan
Copy link
Contributor Author

I was going to say, it feels off balance in one way to correct the author's stray space, but not a stray comma if they left one. But it sounds easy to do.

@rbeezer
Copy link
Collaborator

rbeezer commented Sep 4, 2018

Just reading commits (not the cumulative effect, locally), but I think I have a handle on current state.

Looks fine. But see if you like this better. Basically get commas out of the way early and add back last.

  • Replace all commas in @youtube by spaces.
  • Normalize spaces in this result.
  • Do above in select for a variable $youtube before the choose opens.
  • Now test on $youtube for playlist scenario is simpler and accurate.
  • Replacement is easier, trade spaces for commas in $youtube.

Sorry for the delay, startups have been keeping me busy.

@Alex-Jordan
Copy link
Contributor Author

OK, I was avoiding introducing a variable $youtube. But happy to do that.

The stylesheet could be cleaner if the URL were allowed to be built with an ampersand on the first option. Like https://www.youtube.com/embed?&amp;listType=playlist instead of https://www.youtube.com/embed?listType=playlist. The ?&amp; appeals to me because on those rare occasions when it matters, it is easier to cut and paste the options out of a URL. The only down side is that the simpler ? looks more "human-written". But both function. Do you have an opinion on this?

@Alex-Jordan
Copy link
Contributor Author

LaTeX YT playlist is now here for your first impressions. I haven't actually tested so maybe there is a typo or something. Moving on to the script to get it to scrape first image from a list-defined playlist. Will do a complete test after that.

@Alex-Jordan
Copy link
Contributor Author

As happens, I immediately see issue. Please disregard until I signal the whole thing is tested and ready for review.

@rbeezer
Copy link
Collaborator

rbeezer commented Sep 5, 2018 via email

@Alex-Jordan
Copy link
Contributor Author

Alex-Jordan commented Sep 5, 2018 via email

@Alex-Jordan
Copy link
Contributor Author

OK, tested and ready for review. Perhaps in this order:

  1. Check out the sample-article HTML output here:
    http://spot.pcc.edu/~ajordan/temp/section-video.html
    Figures 16.12 and 16.13.

  2. Check out the PDF output here:
    http://spot.pcc.edu/~ajordan/temp/derivatives.pdf
    Page 67. You will see the issue with the "static-caption" running off its bounded area. This is an issue that is orthogonal to putting in the playlists, but I think the sample-article did not have an example that revealed it. Should it be addressed in this PR? And if so, what would you like to see? URL's that wrap? Just print the IDs?

  3. Examine the PR as a whole. Looking at commits is not so helpful now that there have been multiple commits to each file touched.

  4. Ultimately this will make a thumbnail for the "enumerated" playlist. Should I commit that to the repo like all other youtube thumbnail images for the sample article?

@rbeezer
Copy link
Collaborator

rbeezer commented Sep 6, 2018 via email

@Alex-Jordan
Copy link
Contributor Author

Thumbnail committed. And a bunch of cleanup I somehow missed on my last pass.

@rbeezer
Copy link
Collaborator

rbeezer commented Sep 7, 2018

You will see the issue with the "static-caption" running off its bounded area. This is an issue that is orthogonal to putting in the playlists, but I think the sample-article did not have an example that revealed it. Should it be addressed in this PR? And if so, what would you like to see? URL's that wrap? Just print the IDs?

I need to do more work on static versions of interactive portions, so I'll think more about captions then.

@rbeezer
Copy link
Collaborator

rbeezer commented Sep 7, 2018

Nice work, this is a great enhancement. Corrected one small typo. Otherwise, repackaged as six topical commits, each with your name on it.

Feel free to run your own advertisement on pretext-announce if you think it is time.

Thanks! What's next?

@rbeezer rbeezer closed this Sep 7, 2018
@Alex-Jordan Alex-Jordan deleted the youtube-playlist branch September 14, 2023 03:24
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.

None yet

2 participants