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

develop #2601

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

develop #2601

wants to merge 18 commits into from

Conversation

michal-reszka
Copy link

Copy link

@JoyCoffeeAddict JoyCoffeeAddict left a comment

Choose a reason for hiding this comment

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

  1. Paddings in the navigation are not the same as in the figma
    image
  2. Phone icon on hover does not work
  3. Smooth scrolling is not working
  4. Wrong font-family here
    image
  5. Hovering images does not work
  6. Some styling mistake with a button
    image
  7. Form validation as described in the checklist

Copy link

@mbruhler mbruhler left a comment

Choose a reason for hiding this comment

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

Hi,

On mobile menu close button is in bad position
image

On mobile, Explore button is very small
image

On mobile, grid items are badly positioned
image

On desktop, logo is blurry and logo items are badly positioned
image

Your contact form has extra outline which is not compliant with figma mockup
image

Copy link

@mvjl000 mvjl000 left a comment

Choose a reason for hiding this comment

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

  • form: send button has default borders (remove it), form submit shouldn't reload the page, simply clear the form values, I would disable resize for the textarea,
  • Open menu icon should have the same size as the phone icon (e.g. on the desktop it is way to small),
  • Open/Close menu button should be in the exact same position,
  • On the tablet resolution, the "phone" text shouldn't be higher that the input
    Screenshot 2023-10-27 at 14 49 00

width: 276px;
&:hover {
cursor: pointer;
transition: box-shadow 0.3s;
Copy link

@mvjl000 mvjl000 Oct 27, 2023

Choose a reason for hiding this comment

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

The transitions mustn't be inside a hover. That's why your transitions are not smoothly going back to initial styles when you stop hovering them. Is should be moved up.

Copy link

Choose a reason for hiding this comment

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

To be clear - you should do so for all transitions in order to have them nicely 'animated'

@michal-reszka michal-reszka requested a review from mvjl000 October 27, 2023 15:33
Copy link

@mvjl000 mvjl000 left a comment

Choose a reason for hiding this comment

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

I want you to do something about your hover effects. As I said noticed in the previous review, you are applying transition inside of hover block. It has to be outside of it in order to have smooth 'exit' animation.
Speaking of hover effects, I would like you to prevent the images (for example from 'compare bike') from overlapping on text underneath them.

@michal-reszka michal-reszka requested a review from mvjl000 October 31, 2023 11:18
Copy link

@mbruhler mbruhler left a comment

Choose a reason for hiding this comment

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

Approved, well done!

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.

4 participants