Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src-react/App.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
import { Navigate, Route, Routes } from 'react-router-dom';

import { useSettings } from './shared/settings/SettingsContext';
import Footer from './core/footer/Footer';
import Header from './core/header/Header';
import Feed from './feeds/feed/Feed';
import type { FeedName } from './shared/models';
import './App.scss';

const feedNames: FeedName[] = ['news', 'newest', 'show', 'ask', 'jobs'];

function App() {
const { settings } = useSettings();

Expand All @@ -11,7 +17,16 @@ function App() {
<div className="body-cover" />
<div className="wrapper">
<Header />
<div />
<Routes>
<Route path="" element={<Navigate to="/news/1" replace />} />
{feedNames.map((feedName) => (
<Route
key={feedName}
path={`${feedName}/:page`}
element={<Feed key={feedName} feedType={feedName} />}
/>
))}
</Routes>
<Footer />
</div>
</div>
Expand Down
108 changes: 108 additions & 0 deletions src-react/feeds/feed/Feed.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
@import "../../shared/scss/media";
@import "../../shared/scss/theme_variables";

a {
text-decoration: none;
font-weight: bold;

&:hover {
text-decoration: underline;
};
}

ol {
padding: 0 40px;
margin: 0;

@media #{$mobile-only} {
box-sizing: border-box;
list-style: none;
padding: 0 10px;
}

li {
position: relative;
-webkit-transition: background-color .2s ease;
transition: background-color .2s ease;
}
}

.list-margin {
@media #{$mobile-only} {
margin-top: 55px;
}
}

.main-content {
position: relative;
width: 100%;
min-height: 100vh;
-webkit-transition: opacity .2s ease;
transition: opacity .2s ease;
box-sizing: border-box;
padding: 8px 0;
z-index: 0;
}

.post {
padding: 10px 0 10px 5px;
transition: background-color 0.2s ease;
border-bottom: 1px solid #CECECB;

.itemNum {
color: #696969;
position: absolute;
width: 30px;
text-align: right;
left: 0;
top: 4px;
}
}

.item-block {
display: block;
}


.nav {
padding: 10px 40px;
margin-top: 10px;
font-size: 17px;

a {
@media #{$mobile-only} {
text-decoration: none;
}
}

@media #{$mobile-only} {
margin: 20px 0;
text-align: center;
padding: 10px 80px;
height: 20px;
}

.prev {
padding-right: 20px;

@media #{$mobile-only} {
float: left;
padding-right: 0;
}
}

.more {
@media #{$mobile-only} {
float: right;
}
}
}

.job-header {
font-size: 15px;
padding: 0 40px 10px;

@media #{$mobile-only} {
padding: 60px 15px 25px 15px;
}
}
86 changes: 86 additions & 0 deletions src-react/feeds/feed/Feed.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import { useEffect, useState } from 'react';
import { Link, useParams } from 'react-router-dom';

import ErrorMessage from '../../shared/components/error-message/ErrorMessage';
import Loader from '../../shared/components/loader/Loader';
import type { FeedName, Story } from '../../shared/models';
import { fetchFeed } from '../../shared/api/hackernewsApi';
import Item from '../item/Item';
import './Feed.scss';

interface FeedProps {
feedType: FeedName;
}

function Feed({ feedType }: FeedProps) {
const params = useParams<{ page?: string }>();
const pageNum = params.page ? Number(params.page) : 1;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: Non-numeric page param yields page=NaN request

A non-numeric segment like /news/abc makes Number(params.page) produce NaN, sending ?page=NaN to the API. This matches the Angular original and is not a regression.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

const [items, setItems] = useState<Story[]>();
const [errorMessage, setErrorMessage] = useState('');
const [listStart, setListStart] = useState(0);

useEffect(() => {
let ignore = false;
setErrorMessage('');

fetchFeed(feedType, pageNum)
.then((nextItems) => {
if (ignore) {
return;
}

setItems(nextItems);
setListStart((pageNum - 1) * 30 + 1);
window.scrollTo(0, 0);
})
.catch(() => {
if (!ignore) {
setErrorMessage(`Could not load ${feedType} stories.`);
}
});

return () => {
ignore = true;
};
}, [feedType, pageNum]);
Comment on lines +22 to +45

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Previous feed's stories remain when switching feeds

The fetch effect clears errorMessage but never resets items when feedType changes. React Router reuses the Feed instance across all feed routes, so the previous feed's stories stay visible with no loader until new data arrives; if the new feed fails to load, the stale list stays and the error is hidden.

Suggested change
useEffect(() => {
let ignore = false;
setErrorMessage('');
fetchFeed(feedType, pageNum)
.then((nextItems) => {
if (ignore) {
return;
}
setItems(nextItems);
setListStart((pageNum - 1) * 30 + 1);
window.scrollTo(0, 0);
})
.catch(() => {
if (!ignore) {
setErrorMessage(`Could not load ${feedType} stories.`);
}
});
return () => {
ignore = true;
};
}, [feedType, pageNum]);
useEffect(() => {
let ignore = false;
setItems(undefined);
setErrorMessage('');
fetchFeed(feedType, pageNum)
.then((nextItems) => {
if (ignore) {
return;
}
setItems(nextItems);
setListStart((pageNum - 1) * 30 + 1);
window.scrollTo(0, 0);
})
.catch(() => {
if (!ignore) {
setErrorMessage(`Could not load ${feedType} stories.`);
}
});
return () => {
ignore = true;
};
}, [feedType, pageNum]);
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Real bug, fixing it — but not with setItems(undefined) in the effect. Angular recreated FeedComponent when the route config changed (news → ask) while keeping the component and its list when only :page changed, so clearing in the effect would also blank the list on pagination, which the original didn't do. Adding key={feedName} to the <Feed /> route element reproduces both halves: remount (loader, no stale list) across feeds, state retained across pages.


return (
<div className="main-content">
{!items && !errorMessage && <Loader />}
{!items && errorMessage !== '' && <ErrorMessage message={errorMessage} />}
{items && (
<div>
{feedType === 'jobs' && (
<p className="job-header">
These are jobs at startups that were funded by Y Combinator.
You can also get a job at a YC startup through <a href="https://triplebyte.com/?ref=yc_jobs">Triplebyte</a>.
</p>
)}
{feedType as string !== 'new' && (

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: Preserved quirk: feedType !== 'new' always true

feedType as string !== 'new' is always true since no feed is named 'new', so the <ol> always renders. Matches the Angular original and is called out as an intentionally preserved quirk.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

<ol className={feedType !== 'jobs' ? 'list-margin' : undefined} start={listStart}>
{items.map((item) => (
<li className="post" key={item.id}>
<Item className="item-block" item={item} />
</li>
))}
</ol>
)}
<div className="nav">
{listStart !== 1 && (
<Link to={`/${feedType}/${pageNum - 1}`} className="prev">
‹ Prev
</Link>
)}
{items.length === 30 && (
<Link to={`/${feedType}/${pageNum + 1}`} className="more">
More ›
</Link>
)}
</div>
</div>
)}
</div>
);
}

export default Feed;
68 changes: 68 additions & 0 deletions src-react/feeds/item/Item.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
@import "../../shared/scss/media";
@import "../../shared/scss/theme_variables";

p {
margin: 2px 0;

@media #{$mobile-only} {
margin-bottom: 5px;
margin-top: 0;
}
}

a {
cursor: pointer;
text-decoration: none;
}

.title {
font-size: 16px;
font-family: Verdana, Geneva, sans-serif;
}

.subtext-laptop {
font-size: 12px;
font-weight: bold;
letter-spacing: 0.5px;

a {
&:hover {
text-decoration: underline;
};
}
@media #{$mobile-only} {
display: none;
}
}

.subtext-palm {
font-size: 13px;
font-weight: bold;
letter-spacing: 0.5px;

a {
&:hover {
text-decoration: underline;
};
}

.details {
margin-top: 5px;

.right {
float: right;
}
}
@media #{$laptop-only} {
display: none;
}
}

.domain {
color: #696969;
letter-spacing: 0.5px;
}

.item-details {
padding: 10px;
}
80 changes: 80 additions & 0 deletions src-react/feeds/item/Item.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import { Link } from 'react-router-dom';

import { useSettings } from '../../shared/settings/SettingsContext';
import type { Story } from '../../shared/models';
import { formatComments } from '../../shared/utils/formatComments';
import './Item.scss';

interface ItemProps {
item: Story;
className?: string;
}

function Item({ item, className }: ItemProps) {
const { settings } = useSettings();
const hasUrl = item.url?.indexOf('http') === 0;
const titleStyle = { fontSize: `${settings.titleFontSize}px` };

return (
<div className={className} style={{ marginBottom: `${settings.listSpacing}px` }}>
{hasUrl && (
<p>
<a
className="title"
style={titleStyle}
href={item.url}
target={settings.openLinkInNewTab ? '_blank' : undefined}
rel={settings.openLinkInNewTab ? 'noopener' : undefined}
>
{item.title}
</a>
{item.domain && <span className="domain">({item.domain})</span>}
</p>
)}
{!hasUrl && (
<p>
<Link className="title" style={titleStyle} to={`/item/${item.id}`}>
{item.title}
</Link>
</p>
)}
<div className="subtext-palm">
{item.type !== 'job' && (
<div className="details">
<span className="name">
<Link to={`/user/${item.user}`}>{item.user}</Link>
</span>
<span className="right">{item.points} ★</span>
</div>
)}
<div className="details">
{item.time_ago}
{item.type !== 'job' && (
<Link to={`/item/${item.id}`} className="comment-number">
{' • '}
{formatComments(item.comments_count)}
</Link>
)}
</div>
</div>
<div className="subtext-laptop">
{item.type !== 'job' && (
<span>
{item.points} points by <Link to={`/user/${item.user}`}>{item.user}</Link>
</span>
)}
<span className={item.type !== 'job' ? 'item-details' : undefined}>
{item.time_ago}
{item.type !== 'job' && (
<span>
{' | '}
<Link to={`/item/${item.id}`}>{formatComments(item.comments_count)}</Link>
</span>
)}
</span>
</div>
</div>
);
}

export default Item;