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

ds #508

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

ds #508

wants to merge 45 commits into from

Conversation

jdifek
Copy link

@jdifek jdifek commented Sep 26, 2024

@TarasHoliuk
Copy link

  1. Fix lint errors:

image

  1. It seems like you're trying to hardcode card width (in percent) on the home page here exactly as it is in design:
    image

On design, this card width is an example of how it might look on this screen size. But you can try to adapt it - set 100% and, maybe set some max-width and min-width to handle edge cases and make sure it always looks good.

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.

I see you partially implemented the mobile version. Please, re-request review when the entire task will be done

If you have questions or need help ask in the chat 🙂

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

  1. images in this section don't look right
image
  1. let's fix this case (display the Midnight color not like the white)
image just create the object with colors and reuse it ``` const COLORS = { midnight: #......, ............ } ```
  1. add some favicon and update the title
image
  1. you forgot to implement the pagination
image
  1. display the number of favorite products here
image
  1. looks strange on desktop
image

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.

3 participants