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

layout miami - bose version #2587

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

Conversation

arturgorniak
Copy link

@arturgorniak arturgorniak commented Sep 26, 2023

I didnt notice, in task descripion, requirement to make pull request after every block. I did make commits after every block and significant change but didnt make pull requests. I hope it is not big problem and now i have almost ready site in one go.

DEMO 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.

please, provide the demo link in PR description

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. please, fix all your mismatches with the layout
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.

  1. looks strange
image
  1. all links should have some hovers
image
  1. disable textarea scaling
image
  1. looks different
image image
  1. Add a favicon
  2. Apply :hover effect for images on page (testimonials / gallery, other sections).
  3. change the title
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.

  1. should start the call on click and add some hovers
image
  1. all fields in form should be required
  2. you should stretch the photo at full width
image image

Copy link

@loralevitska loralevitska left a comment

Choose a reason for hiding this comment

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

Keep up the good work! Some comments to fix:

  • make this number reachable, and change color according design
    image
    image

  • change favicon to B&O logo, now you have mateAcademy logo

  • remove outline and default autocomplete background-color, it breaks design
    image

  • make all form fields required

  • add smooth scroll for page, now user navigate to part page immediately from nav menu

  • fix email link, it doesn't work now
    image

  • don't show this number on mobile mode
    image

  • disable scroll while menu is open
    image

  • fix icon indents according design
    image

  • fix this indent according design
    image

@arturgorniak
Copy link
Author

Hello, i need some help. I did some commits and pushed it to the develop branch like usual. Now i cant see new version of github page deployed although it says in vsc that everything went good. Can you tell me what i did wrong? I think i did everthing the same way like all this times befere and i dont know why is it not working :(

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

There are a lot of previous review comments, that are not fixed. Feel free to ask for help in the chat

@arturgorniak
Copy link
Author

There are a lot of previous review comments, that are not fixed. Feel free to ask for help in the chat

I am aware, i fixed it and wanted to deploy gh page and i isnt working and i dont know why :(

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. Add a placeholder text to the textarea
image
  1. Need to remove the autocomplete styles and fix the left indent for the textarea
image
  1. No need to show data from form in the url
image
  1. The color of the phone's text does not match the one on the design
image
  1. Need to remove the scroll when this menu is open
image

@arturgorniak
Copy link
Author

I fixed most of them except for scroling while menu is open but i cant deploy new page. It would help if someboby read this comment instead of asking me to fix the same things again....

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.

Still not fixed from the previous review, if you have any problems feel free in the fe_chat

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.

5 participants