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 #557

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

Develop #557

wants to merge 25 commits into from

Conversation

mrsvolodya
Copy link

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. it should be links
image
  1. cool hovers

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

Good job 👍
Let's improve your page

  1. Change the autofill styles for the form fields
    https://css-tricks.com/snippets/css/change-autocomplete-styles-webkit-browsers/
image
  1. Don't need scroll page to top after submitting form, you need only reset form fields

  2. Add transition for hover effects eerwyhere

image image
  1. Add actual links for these social networks
image
  1. The image in the header should fit to the right side of the browser window. Check it on the layout
image

@mrsvolodya
Copy link
Author

1.done
2. I have done corresponding readme file screen below
(scroll to top after submit) check this please....
image
3.About hover effect I all link this have, check please...
image
image
4. Consider icons link I don't have actual social networks I use only linkedin, in readme written
"Facebook and Instagram icons in the footer should be clickable and open the museum's social networks in a new tab"
but I believe maybe better leave my contact , maybe you suggest better ?

image
5.Done.

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

Hi,

  1. You need to add transition for hover effects everywhere
image You need add transition-duration for hover effects everywhere image
  1. This comment still not fixed from the previous review
image Demo link now image

@mrsvolodya
Copy link
Author

1.Done
2. I have max dimension 1440 px correspond with UI, you mean to stretch to the right edge with full dimension?
My demo: image
Figma:
image

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

On large screen sizes, the image should also fit to the right side of the browser window, as in this section
image
image

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.

lgtm

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