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

Add the YouTube banner information for desktop screens #3282

Merged
merged 1 commit into from Mar 25, 2022

Conversation

dbelokon
Copy link
Contributor

@dbelokon dbelokon commented Mar 22, 2022

Issue This PR Addresses

Fixes #2680

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

This PR adds the front-end for the YouTube banner information. Currently, there are two values that always show negative. This will be addressed in a future PR involving the back-end.

Steps to test the PR

You can view the changes in vercel autodeployment, however, you can do it locally too.

  1. Pull my changes.
  2. Use the staging configuration (cp config/env.staging .env)
  3. Start the front-end (pnpm dev)
  4. Find the earliest post that is a YouTube video (you can search for the one in the screenshots).

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots:
    image
    image
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Mar 22, 2022

Copy link
Contributor

@AmasiaNalbandian AmasiaNalbandian left a comment

Choose a reason for hiding this comment

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

Can we change the text colors and font to match the rest of the GitHub side panel? I believe We need Chanel in blue, and then subscribers/views in the grey text/font?

@JerryHue JerryHue linked an issue Mar 22, 2022 that may be closed by this pull request
Copy link
Contributor

@JerryHue JerryHue left a comment

Choose a reason for hiding this comment

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

So, as we have mentioned in the meeting, we can remove the "Share" button (since it is somewhat ambiguous in what it does for the user).

@Kevan-Y
Copy link
Contributor

Kevan-Y commented Mar 23, 2022

@dbelokon
Step I ran:

  1. gh pr checkout 3282
  2. pnpm i
  3. cp config/env.staging .env
  4. pnpm dev
  5. Go to http://localhost:8000
  6. Search by author Kevan
  7. Find a post with youtube video.
    image

Did I do something wrong? I don't see the youtube banner information.

@AmasiaNalbandian
Copy link
Contributor

Hello! Please rebase, as the following commit 6046273 has reorganized our file structure!

@dbelokon
Copy link
Contributor Author

@dbelokon Step I ran:

  1. gh pr checkout 3282
  2. pnpm i
  3. cp config/env.staging .env
  4. pnpm dev
  5. Go to http://localhost:8000
  6. Search by author Kevan
  7. Find a post with youtube video.
    image

Did I do something wrong? I don't see the youtube banner information.

You didn't do anything wrong, it is just that the YouTube banner is slightly different from the GitHub banner. The main difference is that the YouTube banner will only appear if the post is coming from a YouTube channel feed, and not if the post links to a YouTube video. In my screenshots, I show Dave's YouTube channel which has a feed. If you post a blogpost that links to a YouTube video, that information won't show up because the post itself is a blogpost and not a video post.

The way I designed it is to appear only about posts that are exclusively from a YouTube channel. I think I should file an issue that redesigns this aspect, because I kind of don't want to have to revamp the whole PR again :(

@dbelokon
Copy link
Contributor Author

Just so that everybody is aware, I filed an issue (#3322), that addresses @Kevan-Y's point here:

@dbelokon Step I ran:

  1. gh pr checkout 3282
  2. pnpm i
  3. cp config/env.staging .env
  4. pnpm dev
  5. Go to http://localhost:8000
  6. Search by author Kevan
  7. Find a post with youtube video.
    image

Did I do something wrong? I don't see the youtube banner information.

You didn't do anything wrong, it is just that the YouTube banner is slightly different from the GitHub banner. The main difference is that the YouTube banner will only appear if the post is coming from a YouTube channel feed, and not if the post links to a YouTube video. In my screenshots, I show Dave's YouTube channel which has a feed. If you post a blogpost that links to a YouTube video, that information won't show up because the post itself is a blogpost and not a video post.

The way I designed it is to appear only about posts that are exclusively from a YouTube channel. I think I should file an issue that redesigns this aspect, because I kind of don't want to have to revamp the whole PR again :(

Copy link
Contributor

@JiaHua-Zou JiaHua-Zou 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!

Copy link
Contributor

@Kevan-Y Kevan-Y left a comment

Choose a reason for hiding this comment

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

When scrolling down, the yourubeInfo component is hiding user/org.
image

@dbelokon
Copy link
Contributor Author

When scrolling down, the yourubeInfo component is hiding user/org. image

I actually noticed this when I was writing the PR, and the main problem is how we are currently implementing all the banners. The general banner (the one that shows the profile name) gets covered by the GitHub banner, and the YouTube banner covers the GitHub banner. If I try to fix this, it would make the PR too big, since I would have to also modify the GitHub styling, as well. I can file an issue so we can focus on this only. What do you think?

@Kevan-Y
Copy link
Contributor

Kevan-Y commented Mar 25, 2022

When scrolling down, the yourubeInfo component is hiding user/org. image

I actually noticed this when I was writing the PR, and the main problem is how we are currently implementing all the banners. The general banner (the one that shows the profile name) gets covered by the GitHub banner, and the YouTube banner covers the GitHub banner. If I try to fix this, it would make the PR too big, since I would have to also modify the GitHub styling, as well. I can file an issue so we can focus on this only. What do you think?

OK file an issue on that, and we can merge that

@JerryHue JerryHue merged commit 716d21b into Seneca-CDOT:master Mar 25, 2022
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.

Videos from Youtube feed appear twice Create a YouTube summary information bar for YouTube video posts
5 participants