Skip to content

Conversation

@Sky-A-Fox
Copy link

No description provided.

@crevulus crevulus self-assigned this Jan 3, 2026
@crevulus
Copy link

crevulus commented Jan 3, 2026

@Sky-A-Fox I see you have a commit message that reads "netlify link added", but I can't find the actual link in your PR. Can you please share it?


// Загружаем товары при изменении категории
useEffect(() => {
let isMounted = true;
Copy link

Choose a reason for hiding this comment

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

Can you explain why you need this isMounted flag?

Copy link
Author

Choose a reason for hiding this comment

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

I’ve seen this pattern mentioned as a way to avoid state updates after unmount and potential memory leaks in async effects.
That said, I’m aware that it works outside of React’s lifecycle, doesn’t scale well, and doesn’t look great—especially since it’s used in multiple places.
I agree it would be better to replace it with a proper request cancellation (e.g. AbortController).

const [error, setError] = useState<string | null>(null);

// Используем ref для отслеживания первого рендера
const isInitialMount = useRef(true);
Copy link

Choose a reason for hiding this comment

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

Can you explain why you need this isInitialMount flag?

Copy link
Author

Choose a reason for hiding this comment

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

I used it because I had problem with the logic. But in general we don't need it because it's a crutch.


// Показываем загрузку
if (loading && products.length === 0) {
return <div style={{ padding: '20px', textAlign: 'center' }}>Loading products...</div>;
Copy link

Choose a reason for hiding this comment

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

You use inline styles here and css stylesheets elsewhere. For consistency, it's best to use one or the other.

const loadCategories = async () => {
try {
const response = await fetch("https://fakestoreapi.com/products/categories");
if (!response.ok) throw new Error('Failed to load categories');
Copy link

Choose a reason for hiding this comment

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

If you throw a new error in your try block, the message from the error gets passed to the catch block. In your implementation, this means you show the same error mssage twice.

Uploading Screenshot 2026-01-04 at 16.28.14.png…

const loadCategories = async () => {
try {
const response = await fetch("https://fakestoreapi.com/products/categories");
if (!response.ok) throw new Error('Failed to load categories');
Copy link

Choose a reason for hiding this comment

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

If you throw a new error in your try block, the message from the error gets passed to the catch block. In your implementation, this means you show the same error mssage twice.

Screenshot 2026-01-04 at 16 28 14

Copy link

@crevulus crevulus left a comment

Choose a reason for hiding this comment

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

Great work, Ivan!

You have made good fetching code - just what we need. You may have overengineered a bit - be careful not to copy & paste code that you don't necessarily need. Also please delete unused files (e.g. App.css) to keep your files clean for your colleagues.

Also your PRs should include only the files that are relevant to this week, not last week's code. Please make sure you keep your PRs lean - the same will be true at the company you join!

@Sky-A-Fox
Copy link
Author

@Sky-A-Fox I see you have a commit message that reads "netlify link added", but I can't find the actual link in your PR. Can you please share it?

my bad forget to push.
the link in dress project => readme.md (file nearby folder with project)
https://github.com/Sky-A-Fox/React-Cohort54/blob/IVAN-w2-REACT/week2/project/README.md

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