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

Make Youtube link clickable #2706

Merged
merged 1 commit into from
Feb 2, 2022
Merged

Conversation

tpmai22
Copy link
Contributor

@tpmai22 tpmai22 commented Jan 22, 2022

Issue This PR Addresses

Fixes #2679

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

Make Youtube link clickable by wrapping <a> tag .

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: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • 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)

Testing

At the root directory remove redis-data

rm -r redis-data

Rebuild images and wait for redis to cache the feeds

image

@gitpod-io
Copy link

gitpod-io bot commented Jan 22, 2022

@humphd
Copy link
Contributor

humphd commented Jan 22, 2022

This doesn't solve the issue. What #2679 is about is taking the raw text in a YouTube description, and parsing it to find any URLs, then turning that into HTML that wraps them in <a> elements, so you can click them.

There are some existing React components for doing this, e.g., https://www.npmjs.com/package/react-linkify.

@tpmai22
Copy link
Contributor Author

tpmai22 commented Jan 23, 2022

@AmasiaNalbandian for the update I still working on it right now

@humphd
Copy link
Contributor

humphd commented Jan 23, 2022

Thinking about this more, we should probably use a JS lib vs. React Component for this, maybe https://www.npmjs.com/package/linkifyjs.

@tpmai22 tpmai22 added this to the 2.6 Release milestone Jan 26, 2022
@tpmai22
Copy link
Contributor Author

tpmai22 commented Jan 27, 2022

@humphd I suppose to test if this we will only need to run pnpm dev at the root directory of our project ? Or do i need to rebuild any image if I'm using Gitpod . Since I have tried to make some changes but everytime I ran into your Youtube posts it still render the same so I'm just try to confirm pnpm dev is the right way to test it out

@humphd
Copy link
Contributor

humphd commented Jan 27, 2022

I'm not sure what you're doing, I need more context.

@tpmai22
Copy link
Contributor Author

tpmai22 commented Jan 27, 2022

After wrapping the description with react-linkify I try to test them by running only frontend using pnpm dev but everytime I scroll to your Youtube post the description still display at plain text so I wonder if I need to rebuild the post image so as to make my changes to be applied ?

<section
ref={sectionEl}
className={`telescope-post-content ${extractBlogClassName(post.url)}`}
dangerouslySetInnerHTML={{ __html: post.html }}
/>

@humphd
Copy link
Contributor

humphd commented Jan 27, 2022

As I mentioned, don't use a React component for this. Use a JS library that does it. It can happen in the front-end (fine) or back-end (ideal).

@humphd
Copy link
Contributor

humphd commented Jan 27, 2022

For example, https://www.npmjs.com/package/linkify-it

src/api/posts/src/data/post.js Outdated Show resolved Hide resolved
@@ -148,18 +148,26 @@ class Post {
article.date = article.pubdate;
}

if (Array.isArray(article.content) && determinePostType(article.link) === 'video') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we adding this? When is the content an Array vs. a string, and why have we never hit this in the past (e.g., with @dbelokon's previous work on YouTube)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I did debugging all the of the Youtube post return article.content as an array off object instead of a string like what we expected which make the html content of all the related video post not being proccesed

if (typeof html !== 'string') {
return html;

image

src/backend/data/post.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/backend/data/post.js Outdated Show resolved Hide resolved
src/backend/utils/html/index.js Outdated Show resolved Hide resolved
@tpmai22 tpmai22 added this to the 2.6 Release milestone Feb 2, 2022
@aserputov
Copy link
Contributor

I do not mind, but the deadline for 2.6 is till midnight. Let's ask David about this tomorrow.

@humphd
Copy link
Contributor

humphd commented Feb 2, 2022

This doesn't depend on microservices. We just need a couple of tests that include a post with text that doesn't have <a> tags, and to make sure it gets linkified when it goes through the processor code.

I'm not sure what the question/hold-up is? Let me know.

@tpmai22
Copy link
Contributor Author

tpmai22 commented Feb 2, 2022

@humphd I did add the test and also I want to help moving my code to the parser but I wonder if I'm doing it correct so i'm watiing for @TueeNguyen to give me an idea on how to test it. Otherwise like usual I'm waiting for your suggestion on how to improve my code (do I need to add more test, etc ...)

@TueeNguyen
Copy link
Contributor

TueeNguyen commented Feb 2, 2022

I think let's not change the parser service, it is not up to date now. I'll take your changes from backend to my branch

@tpmai22
Copy link
Contributor Author

tpmai22 commented Feb 2, 2022

@TueeNguyen could you do ma a favor and ping me when you move it to the parser ? I just want to see how we implement that

@TueeNguyen
Copy link
Contributor

@TueeNguyen could you do ma a favor and ping me when you move it to the parser ? I just want to see how we implement that

@tpmai22 https://github.com/TueeNguyen/telescope/tree/feed-parser

src/backend/data/post.js Show resolved Hide resolved
test/post.test.js Show resolved Hide resolved
Copy link
Contributor

@TueeNguyen TueeNguyen left a comment

Choose a reason for hiding this comment

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

Can you rebase to add the bug fix #2795? So that we can check if PR passes tests

@aserputov aserputov merged commit 4cec3c2 into Seneca-CDOT:master Feb 2, 2022
TueeNguyen pushed a commit to TueeNguyen/telescope that referenced this pull request Feb 6, 2022
- Initial work from @manekenpix
- Edited `npm run dev`
- Export Redis constructor instead
- Added changes based on backend and Seneca-CDOT#2706 Seneca-CDOT#2581
- Bumped node-fetch to 2.6.7
@TueeNguyen TueeNguyen mentioned this pull request Feb 6, 2022
8 tasks
TueeNguyen pushed a commit to TueeNguyen/telescope that referenced this pull request Feb 10, 2022
- Initial work from @manekenpix
- Edited `npm run dev`
- Export Redis constructor instead
- Added changes based on backend and Seneca-CDOT#2706 Seneca-CDOT#2581
- Bumped node-fetch to 2.6.7
TueeNguyen pushed a commit to TueeNguyen/telescope that referenced this pull request Feb 10, 2022
- Initial work from @manekenpix
- Edited `npm run dev`
- Export Redis constructor instead
- Added changes based on backend and Seneca-CDOT#2706 Seneca-CDOT#2581
- Bumped node-fetch to 2.6.7
TueeNguyen pushed a commit to TueeNguyen/telescope that referenced this pull request Feb 12, 2022
- Initial work from @manekenpix
- Edited `npm run dev`
- Export Redis constructor instead
- Added changes based on backend and Seneca-CDOT#2706 Seneca-CDOT#2581
- Bumped node-fetch to 2.6.7
TueeNguyen pushed a commit to TueeNguyen/telescope that referenced this pull request Feb 12, 2022
- Initial work from @manekenpix
- Edited `npm run dev`
- Export Redis constructor instead
- Added changes based on backend and Seneca-CDOT#2706 Seneca-CDOT#2581
- Bumped node-fetch to 2.6.7
TueeNguyen pushed a commit to TueeNguyen/telescope that referenced this pull request Feb 15, 2022
- Initial work from @manekenpix
- Edited `npm run dev`
- Export Redis constructor instead
- Added changes based on backend and Seneca-CDOT#2706 Seneca-CDOT#2581
- Bumped node-fetch to 2.6.7
TueeNguyen pushed a commit to TueeNguyen/telescope that referenced this pull request Feb 15, 2022
- Initial work from @manekenpix
- Edited `npm run dev`
- Export Redis constructor instead
- Added changes based on backend and Seneca-CDOT#2706 Seneca-CDOT#2581
- Bumped node-fetch to 2.6.7
TueeNguyen pushed a commit to TueeNguyen/telescope that referenced this pull request Feb 16, 2022
- Initial work from @manekenpix
- Edited `npm run dev`
- Export Redis constructor instead
- Added changes based on backend and Seneca-CDOT#2706 Seneca-CDOT#2581
- Bumped node-fetch to 2.6.7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrap URL links into <a> elements in YouTube descriptions
7 participants