Design comparison
Solution retrospective
Setting navigational menu was difficult for me . I think my javascript code is not optimal it does work but there must be a way to do it better. any suggestions will be appreciated.
Community feedback
- @NJVSPosted over 2 years ago
Hi, Abdul. Congrats for completing the challenge.
Regarding your dropdowns, I've notice that the
<li id="features">
and<li id="company">
has the same click event. You can just assign a identical class name for your dropdown like this<li class="dropdown">
then select both of them in javascript then run aforEach()
method then add click event(thats a lot of "then" LOL). Also, instead of toggling class forsub-menu
,<a>
and<i class="fa-solid">
, I suggest to just toggle a class on their parent element.const dropdowns = document.querySelectorAll("li.dropdown"); dropdowns.forEach(dropdown => { dropdown.addEventListener("click", function(event) { event.currentTarget.classList.toggle("active"); }); });
.dropdown.active > a { font-weight: 700; } .dropdown.active > a i.fa-solid { transform: rotate(180deg); } .dropdown.active > .sub-menu { display: block; }
Marked as helpful1@abymaniPosted about 2 years ago@NJVS Thanks a lot. I have learned something new from you. you are awesome. :)
1 - @JetyunPosted over 2 years ago
- the Accessibility issue, you can put header tag to contain all the tag that is not in the main tag.
- for HTML, i can only comment on the img tag issue, where you no need to put unit on the width and height (the system knew that it is pixel)
on design:
-
I think you missed the active state on certain button (career, about, login, register and learn more)
-
On the greyed out section in the mobile view, i actually used a div with position: absolute with background color of almost black paired with opacity setting to recreate the design. the div, you can just put above the big picture, no need to put anything in the div tag. Then just use JS to amend the display between none and block when you clicked the menu icon. remember to set the width and height of the div so that the grey area actually covered the same area in the design.
Marked as helpful0 - @freakyjonesPosted over 2 years ago
Hi Abdul,
congrats on completing the challenge
I just saw your code, I would like to give some suggestions with your permission
- start using data attributes in your code, It will be much more efficient to change open and hide the nav and reduce the use of excessive javascript.
- try using a z-index while stacking multiple components in one place . Here is one of my favorite videos regarding z-index (https://www.youtube.com/watch?v=uS8l4YRXbaw).
hope it helps, Thanks Happy coding :)
0
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord