Conversation
natalia-klonowska
left a comment
natalia-klonowska
left a comment
There was a problem hiding this comment.
background image is not displayed. check if file path is correct
There was a problem hiding this comment.
use only full versions for better quality. This applies to all images so make sure to update all of them
index.html
Outdated
| <link rel="preconnect" href="https://fonts.gstatic.com" | ||
| crossorigin="anonymous"> | ||
| <link href="https://fonts.googleapis.com/css2?family=Poppins:ital,wght@0,100;0,200;0,300;0,400;0,500;0,600;0,700;0,800;0,900;1,100;1,200;1,300;1,400;1,500;1,600;1,700;1,800;1,900&display=swap" rel="stylesheet"> | ||
| <link rel="stylesheet" href="/css/styles.css"> |
There was a problem hiding this comment.
you are allowed to link *.scss files directly in HTML tags using href attribute. This is possible because we use the Parcel library to bundle your solution's source code
natalia-klonowska
left a comment
natalia-klonowska
left a comment
There was a problem hiding this comment.
-
navigation links change size on hover causing shift in position of other links
https://www.loom.com/share/76e30a57b4fd4af7bb81d8ad299fc53c -
layout is not responsive it's fixed with breakpoints which results in too large margins between them:
Here you can find a comparison between fixed and responsive layouts and here you can see example of responsive layout with breakpoints
Zibi95
left a comment
Zibi95
left a comment
There was a problem hiding this comment.
Hey good job so far. Here are some things that caught my eye.
Some decisions are questionable and hard to maintain.
For example:
- Here you have two separate text and you are using that main grid to position them
Instead you could just create some wrapper and position them with flex and then put some margin on that wrapper instead of some mix of padding and margin on childs.
- Also, there’s a mix of margin-bottom, padding-top, and grid-gap calculations involved, so it’ll be a pain to revisit and adjust later.
That bg on hover is awful, lets just stick with that size increase.
- This phone number should no break into new line
- That bike in the background of hero doesnt match what figma is showing. Its to big. Maybe you can create some smaller versions of this image. Also it would be good to to compress the image to lower its size https://tinypng.com/
Zibi95
left a comment
Zibi95
left a comment
There was a problem hiding this comment.
Content is getting to big at bigger screens and images starts to look ugly. Apply some max-width for the content on figma i saw that its around 1020px for content plus that padding 120px on left and right. You can apply those and just center it with margin: 0 auto;
Zibi95
left a comment
Zibi95
left a comment
There was a problem hiding this comment.
That width resctrication also should apply to the nav
and here to that title
Missing spacing here
4 participants
[x] [DEMO LINK]: https://botqqku.github.io/layout_miami/