-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 #2589
base: master
Are you sure you want to change the base?
Develop #2589
Conversation
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 did an amazing job! Looks good! Let's do some improvements
- add transition for both sides (use this property for element, not just for hover)
- make all form field required
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.
It was a pleasurable experience to interact with your page! I like smooth hovers and high-quality images here. The layout for mobiles and desktops is awesome
Consider fixing validation errors for proper rendering in the browser
The menu in landscape mode should be scrollable (use checklist - 18)
The container for tablets should be wider to meet the Figma design
I figured out how to reset form fields without refreshing whole site. That's why I did another commit after I requested a review :) |
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.
Awesome work! You have done a lot of work here! 💪
DEMO LINK