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

Show videos in the main timeline #2596

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

dbelokon
Copy link
Contributor

Issue This PR Addresses

#1026: This will address the front end implementation for viewing videos in Telescope

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

Display videos on the main timeline, processed by feed of a YouTube channel. This depends on PR #2581 to display any videos at all.

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR does not need tests because it changes the UI.
  • Screenshots: image
  • Documentation: This PR does not documentation, as it adds UI changes.

@gitpod-io
Copy link

gitpod-io bot commented Dec 11, 2021

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very cool. One other thing I'd like to see, either here or in follow-up, is to make links into HTML <a> elements, so they are clickable. I can't remember the subset of HTML that YouTube lets you put in the description, but it's pretty small, and we could likely write a simple parser for it.

src/web/src/components/Posts/Post.tsx Show resolved Hide resolved
src/web/src/components/Posts/Post.tsx Outdated Show resolved Hide resolved
pnpm-lock.yaml Outdated Show resolved Hide resolved
@dbelokon
Copy link
Contributor Author

@humphd, would it be possible to modify the feed list in the CDOT wiki to include your YouTube channel? I would like to see whether the back-end with the front-end would display the video.

@humphd
Copy link
Contributor

humphd commented Jan 19, 2022

@humphd, would it be possible to modify the feed list in the CDOT wiki to include your YouTube channel? I would like to see whether the back-end with the front-end would display the video.

Done. I added this:

[http://blog.humphd.org/tag/seneca/rss/]
name=David Humphrey

[https://www.youtube.com/feeds/videos.xml?channel_id=UCqaMbMDf01BLttof1lHAo2A]
name=David Humphrey (YouTube)

@dbelokon
Copy link
Contributor Author

One other thing I'd like to see, either here or in follow-up, is to make links into HTML elements, so they are clickable.

I will submit an issue to address this.

@dbelokon
Copy link
Contributor Author

I checked the preview deployment after @humphd added his YouTube feed to the CDOT wiki feed list. Here are the results:

PC screens:

image

Phone screens:

image

This is on the screen resizing of the browser, so on an actual phone it might be different. It can be improved, but I am not sure how we would like to show it for mobile user (maybe with a redirect to the YouTube mobile app if they have it?).

Let me know of any other small changes I may do.

cc @humphd, @DukeManh, @cindyledev

@humphd
Copy link
Contributor

humphd commented Jan 19, 2022

Awesome! In a follow-up, we should add a side-bar that includes YouTube links and the YouTube icon, like we do with GitHub info. @dbelokon can you file that for me please?

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a couple small things.

src/web/package.json Outdated Show resolved Hide resolved
src/web/src/pages/_app.tsx Outdated Show resolved Hide resolved
src/web/src/components/Posts/Post.tsx Outdated Show resolved Hide resolved
src/web/src/interfaces/index.ts Outdated Show resolved Hide resolved
humphd
humphd previously approved these changes Jan 19, 2022
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R+ with CI/CD passing.

@DukeManh
Copy link
Contributor

Great work on implementing both backend and frontend.

nit: I think the video could expand to full width on Mobile as the screen is smaller.

redirect to the YouTube mobile app if they have it?

I tested this on my phone, clicking on the video title does redirect to the youtube app. We can still watch in the browser directly.

humphd
humphd previously approved these changes Jan 19, 2022
DukeManh
DukeManh previously approved these changes Jan 19, 2022
@dbelokon
Copy link
Contributor Author

@TDDR, since the reviews approved the changes and the PR is done, I'll be rebasing and merging once the CI checks have completed.

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.

3 participants