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

Created a reddit-card component #262

Merged
merged 3 commits into from
Oct 2, 2022
Merged

Conversation

Malay-dev
Copy link
Contributor

I created a new card that displays the top 10 posts from Reddit from the past week.

test-3

It displays the number of upvotes, user, date, image, and title on which onClick opens the Reddit post.
Closes issue #254

Copy link
Member

@petrvecera petrvecera left a comment

Choose a reason for hiding this comment

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

Looks awesome 👍 There are just 2 little things we can change before merge. :)

Otherwise good job.

const [fetched, setFetched] = useState(false);
const data = async () => {
const res = await axios.get(
"https://www.reddit.com/r/CompanyOfHeroes/top.json?limit=50&t=week",
Copy link
Member

Choose a reason for hiding this comment

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

Are we able to swap axios with fetch ? That we we don't need to include extra lib

I think code like this should work:

    const response = await fetch("https://www.reddit.com/r/CompanyOfHeroes/top.json?limit=50&t=week" );
   const responseData = await response.json()
   setpostData(responseData?.data?.children);

Fetch is modern request lib in native JS https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch
Now it's finally also in NodeJS (but only 18+)

So we don't need to include axios in the web projects anymore.

.filter(function (e: any) {
return e?.data?.link_flair_text === "CoH2";
})
.slice(0, 10)
Copy link
Member

Choose a reason for hiding this comment

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

I would move the filter and slice into the data function. In case something would trigger re-render of the component. These (filter and slice) would be run too. When we move it , it won't be run because it will be already saved in the state.

@petrvecera
Copy link
Member

And don't forget to run yarn fix before the commit. It will format your code.
image

@Malay-dev
Copy link
Contributor Author

I have made the requested changes, please re-review the PR @petrvecera.

@Malay-dev Malay-dev requested a review from petrvecera October 2, 2022 05:46
Copy link
Member

@petrvecera petrvecera left a comment

Choose a reason for hiding this comment

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

Awesome

@petrvecera petrvecera merged commit 459d2a8 into cohstats:master Oct 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants