@asbhogal
Posted
Hi Feng,
Great job! Your design matches the mockups nicely. I've just noticed a few things that are worth considering:
- Firstly, its great to see you've locally hosted your fonts, however these should be converted to
woff2format because they're designed for the web (greater accessibility etc.) Here's a link explaining in further detail from W3, who establishes web standards Link - Your images should be inside a
<picture>element with paths to the mobile and desktop versions which show depending on the viewport width. Here's a link from the MDN website on how to achieve this Link - With regards to your JS, you can refactor this by using
.toggle()to switch between the different class names, add event listeners to the parent containers and use separate functions to manage the feature and company dropdown toggling. The simpler option would be to change the classes for these, have these set to hidden, with a class for toggling thedisplaywhich is then dynamically changed using the event listeners. - In terms of HTML, your nav items also need to be semantically changed - they should be in a
<header>with the<nav>child class, with a<ul>and then<li>for each item. Remove thearticleas this is semantically incorrect - Your
font-sizeshould always be inremand notpxto respect the root font size of the user's browser, which is important for accessibility. Here's a good article explaining this in further detail Link - Avoid using
idfor elements as these cause issues when applying styles (as they have a higher specificity.) Also, SCSS allows you to nest declarations, including hovers. For eg.,
#menu { height: 1rem; display: flex; flex-direction: column; justify-content: space-between; float: right; margin: 1rem; z-index: 1.5px; }
#menu:hover { cursor: pointer; transform: scale(1.08, 1.08); transition: 0.5s; }
Can be refactored to:
#menu { height: 1rem; display: flex; flex-direction: column; justify-content: space-between; float: right; margin: 1rem; z-index: 1.5px; &:hover { cursor: pointer; transform: scale(1.08, 1.08); transition: 0.5s; } }
- You will also benefit from using features like
@mixinsto store declarations which you can use on different elements without repeating them. eg. instead of applyingdisplay: flex; flex-direction: column; justify-content: space-between;to multiple elements that need it, you can create a mixin, e.g.@mixin flex-columnwith these values then reference them in your elements by using@include flex-column. Here's a playlist from Kevin Powell on YouTube which explains this further Link
Hope this helps!
Marked as helpful
@Feng-09
Posted
@asbhogal Thank you so much for these tips. I'll make sure to put these to practice and get used to them. Not sure I fully understand what you said regarding my JS though; why should I use separate functions for the feature and company drop-down toggling and by toggling the display, do you mean display: none;?
@asbhogal
Posted
@Feng-09 Hi Feng, sorry for the late reply. That's one way you can do it with the display properly. With the JS, its generally best to use the simplest code and let CSS handle certain aspects where it can, e.g. instead of animating the menu lines in JS, use CSS animations of the lines within a separate class which toggles on and off (ie. is appended to, then removed from, the toggle element, in this case your hamburger menu) which are attached using event listeners. Here's a link to an old repo which hopefully explains this (it's probably easier to see this in context than send a chunk of new code.) Link
Hope this helps!
Marked as helpful
