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

Bose layout #2655

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

Bose layout #2655

wants to merge 9 commits into from

Conversation

WGoliasz
Copy link

Unfortunately, my project does not contain commits because at the end of the project I encountered a critical error that forced me to delete the entire directory and create a new one to which I moved the entire src folder.

[DEMO LINK] (https://WGoliasz.github.io/layout_miami/)

Copy link

@BogdanMaliuta BogdanMaliuta left a comment

Choose a reason for hiding this comment

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

Hello, here some things to fix in your landing:
your burger menu, must be on this page,not another
image
Why its green?
image
Text must be in 1 line
image
Text must be bold
image
Look at font
image
Also check your mobile and tablet version.
Good luck!

@WGoliasz
Copy link
Author

I corrected all errors except one, I don't understand what you mean -"burger menu, must be on this page, not another" - could you explain to me?

@WGoliasz WGoliasz requested a review from BogdanMaliuta April 23, 2024 20:25
Copy link

@BogdanMaliuta BogdanMaliuta left a comment

Choose a reason for hiding this comment

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

Hello here some things to fix
tablet version:
your left and right positions of logo, burger etc.
image
fix it according to design
image
do same to all another page componets, also check your mobile version

@WGoliasz
Copy link
Author

Sorry, I didn't arrange the elements on the page carefully because I thought not to make it "pixel perfect".
Zrzut ekranu 2024-04-24 115317

@WGoliasz
Copy link
Author

I think it will be fine now :)

@WGoliasz WGoliasz requested a review from BogdanMaliuta April 24, 2024 10:02
Copy link

@yevhenii-pyl yevhenii-pyl left a comment

Choose a reason for hiding this comment

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

Solid work and definitely a good amount of effort put it here! 👍

One thing I really want to notice here: you tend to use margin-top && padding-top to position your elements. And you do it for all elements consistently. Great job!
It's a valid approach and if you started using -top property to position elements - you should stick to it and never switch to randomly using -bottom.

Also, you could mark your sections as rows and position them using row-gap property. but that would be just another approach. Yours is also valid and do the job.

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