@rfilenko
Posted
Hey, good job, with smaller adjustments you can really improve this solution. Here are few issues I found:
- don't set width with vw on body, that causes issues;
- try to use more semantic html tags;
- use classes for styling instead of id's;
- more consistency with values (margin-left: 11.7rem; padding-left: 10%; margin-top: 2.5rem;padding: 0.2rem 0.9rem;)
- buttons issue ( use max-width to constrain they or define flexbox on parent div and adjust position);
- setting .logo width: 15% causing it to grow way to big on desktop, use just px or rems;
- would make company-links a ul list.
Start with fixing those first😉
Keep practicing, Roman
@evannor
Posted
Thank you so much for your response Roman!! Your's is one of the most helpful ones I've recieved so far. If you have a moment to clarify in regard to consistency with values, do you mean that I'm using margin and padding ineffectively, or that some values are rem while others are percentages? I try to only use percentages when I want the size to be dynamic based on screen size.
Thank you again for taking the time to review my code!
@rfilenko
Posted
@evannor Hi, I'm pleased to hear my feedback was really helpful to you. What I meant with consistency - on bigger projects usually there is some kind of design system, where there is a defined, scaled system of values. Great approach to use points for measurements like 4px, 8px, 16px, 32 px and so on (or proper values in ems, rems). This way all spacings, sizes between elements will be consistent and scalable. Hope this makes sense.
Have a good day, Roman
