-
Notifications
You must be signed in to change notification settings - Fork 7
Ivan w2 react #16
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
base: main
Are you sure you want to change the base?
Ivan w2 react #16
Conversation
|
@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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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>; |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const loadCategories = async () => { | ||
| try { | ||
| const response = await fetch("https://fakestoreapi.com/products/categories"); | ||
| if (!response.ok) throw new Error('Failed to load categories'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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!
my bad forget to push. |

No description provided.