Create Mobile Toggle Menu with JavaScript #9
Conversation
Ol-create
commented
Mar 8, 2022
•
Ol-create
commented
- Initialize toggle menu in dom.js
- Create a show function for the toggle menu.
- Create hide function for toggle menu.
- Link menu option to a specific section on the portfolio page.
- Add a close button icon.
- Add eventListeners to close and openMenu buttons.
There was a problem hiding this comment.
STATUS: CHANGES REQUIRED ♻️
Hi @Ol-create 👋,
Good job so far!
There are some issues that you still need to work on to go to the next project but you are almost there!
To Highlight 🎉
- No linter errors. ✔️
- GitHub flow is followed. ✔️
- Professional README file. ✔️
- PR has a good title and summary. ✔️
Required Changes ♻️
Check the comments under the review.
Optional suggestions
Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.
You can also consider:
N/A
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
| <nav id="navbar"> | ||
| <p id="profileName"><a href="#"> Paul, Oluyemi</a></p> | ||
| <ul id="desktop-nav"> |
There was a problem hiding this comment.
- You did a good job on the project, but there is a little issue. You are supposed to the figma design as much as possible, Right now your header doesn't contain 100% of the width and there is white space to the right side, Please make sure your header contains 100% of the width and it matches the design on the template.
Your design
Figma design
| <nav id="navbar"> | ||
| <p id="profileName"><a href="#"> Paul, Oluyemi</a></p> | ||
| <ul id="desktop-nav"> |
There was a problem hiding this comment.
- Your mobile-menu pop-up have some margin from the bottom and from the right, Please make sure it contains 100% width and height of the window and matches the design on the template.
Your design
Figma design
| <nav id="navbar"> | ||
| <p id="profileName"><a href="#"> Paul, Oluyemi</a></p> | ||
| <ul id="desktop-nav"> |
There was a problem hiding this comment.
- Your close X button color is different from the design, Please make sure you apply the correct color and it matches the design.
- Your mobile menu options should be bold as on the design, Please use bold font for your mobile menu options and make sure it matches the design on the template.
Your design
Figma design
| <nav id="navbar"> | ||
| <p id="profileName"><a href="#"> Paul, Oluyemi</a></p> | ||
| <ul id="desktop-nav"> |
There was a problem hiding this comment.
- When I open the mobile menu and switch to the desktop screen the mobile menu is still on the screen it should disappear. Please make sure when we switch the screen from mobile to desktop the mobile menu should disappear.
- You can use media queries for that if the screen cross the min-width you can set your mobile menu
display: none
See the image for reference
There was a problem hiding this comment.
Hi, @Ol-create.
Great work on making the changes requested by a previous reviewer 👏
You've done well implementing some of the requested changes, but there are still some which aren't addressed yet.
Required Changes ♻️
- Check the comments under the review.
Optional suggestions
- N/A
Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will really improve your application.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
style.css
Outdated
| #toggle-menu { | ||
| display: none; | ||
| position: fixed; | ||
| z-index: 2; | ||
| top: 0; | ||
| left: 0; | ||
| width: 100%; | ||
| height: 100vh; | ||
| background-color: white; | ||
| border: 1px solid #6f6c6b; | ||
| } | ||
|
|
There was a problem hiding this comment.
-
Please, consider that your mobile menu still does not cover Figma design requirements, as was mentioned by the previous reviewer, but you are one step away from the finish line 👍 Here you can see, that the menu is not cover the whole page and you can see how is the background page content is scrolling:
-
My first suggestion is to give 100% for the height and go-go with your own modifying to be closer to Figma design 👍
There was a problem hiding this comment.
Hi @Ol-create,
Your project is complete! There is nothing else to say other than... it's time to merge it 
Congratulations! 🎉
Optional suggestions
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
