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

react catalog #181

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

katerynashylina
Copy link

@TarasHoliuk
Copy link

The home page link leads to a 404 page (on other pages as well):
image

Align content on the product page according to the design (also, it'd be nice for you to make sure it looks good on screens with 1920px width or more, not only 1440px. Most likely it will be open (by HRs) from laptops with such resolution):
image

Copy link

@TarasHoliuk TarasHoliuk left a comment

Choose a reason for hiding this comment

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

Very good job! Looks amazing 🔥
I added a few comments - check them

</h2>

<p className="image__subtitle">
71 models

Choose a reason for hiding this comment

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

it's better to use dynamic value

Comment on lines 62 to 69
Screen
{' '}
<br />
{' '}
Capacity
{' '}
<br />
{' '}

Choose a reason for hiding this comment

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

use string template

src/App.tsx Outdated
Comment on lines 25 to 26
const [likedProducts, setLikedProducts] = useState<Phone[]>([]);
const [cartProducts, setCartProducts] = useState<CartItem[]>([]);

Choose a reason for hiding this comment

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

I'd recommend you create useLocalStorage custom hook to handle values stored in the local storage instead of saving them here and passing them as props to child components
you may google it or ask in the chat if you need help

Copy link

@TarasHoliuk TarasHoliuk left a comment

Choose a reason for hiding this comment

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

Well done 👍 Added one comment - fix it (easy to do, I'm sure you will do it right - so approved)

Comment on lines 34 to 36
{phones.length}
{' '}
models

Choose a reason for hiding this comment

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

Suggested change
{phones.length}
{' '}
models
{`${phones.length} models`}

fix all similar cases
the variant with string template looks much laconic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants