Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Portfolio mobile menu #10

Merged
merged 23 commits into from
Jun 28, 2023
Merged

Portfolio mobile menu #10

merged 23 commits into from
Jun 28, 2023

Conversation

the-faizmohammad
Copy link
Owner

Pull request Summary

  • This pull request enhances the existing code related to the mobile menu functionality.

  • The changes ensure that the mobile menu appears when the hamburger button is clicked and disappears when the close button (X) is clicked or when any of the mobile menu options are clicked.

  • Additionally, the code correctly handles the display of the corresponding part of the page when a mobile menu option is selected.

  • Updated the READme file.

The following modifications were made:

  1. Consolidated the event listeners for opening and closing the mobile menu into a single function called toggleMobileMenu().

  2. Attached the toggleMobileMenu() function as the event handler for both the hamburger button (navShow) and the close button (navHide).

  3. Added an event listener to each mobile menu option (mobMenuLinks) to prevent the default behavior and ensure the mobile menu disappears when an option is clicked.

  4. Ensured that the appropriate part of the page is displayed when a mobile menu option is selected.

Copy link

@topeogunleye topeogunleye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @the-faizmohammad, @binyamolango, @MohammadYaser,

While you made a great effort in this project, unfortunately, I cannot proceed to review your code.

Invalid Code Review Request

Invalid Code Review Request🛑

  • Kindly update your linters.yml file for this project to include ESLinters. In the root project, you have a folder named .github. Inside this one, you have another folder named workflows. And inside the workflows folder, you have a file named linters.yml. Inside this file, you need to update the linters. The linters needed for this project can be found here;
Desp Images
Actions without ESLint image
Actions with ESLint image

Your Code Review Request will be marked as invalid in your Dashboard, so please submit a new one once you are ready 🙏

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.

Invalid Code Review Request does not count into the code reviews limit.

Copy link

@taldr27 taldr27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status: Approved! 🟢

Hi @the-faizmohammad

Your project is complete! There is nothing else to say other than... it's time to merge it :shipit:
Congratulations! 🎉 🎉

congrats

Highlights 🎉

✅ Professional and customized README. 🤓
✅ Linters tests passing. 👍
✅ Correct use of GitHub Flow. 🙌
✅ Great job using JavaScript to show your menu. 💯

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.


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 @taldr27 in your question so I can receive the notification.

If you need any further help or have any questions, feel free to reach out to me on Slack as Diego Garcia.🎯


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. gif

Comment on lines +49 to +65
.mobile-menu--show {
display: flex;
flex-direction: column;
position: absolute;
top: 0;
left: 0;
padding: 1em 0.75em;
width: 100vw;
height: 100vh;
color: #fff;
background: rgba(96, 112, 255, 0.9);
font-family: "Poppins", sans-serif;
font-weight: 600;
font-size: 2rem;
line-height: 44px;
backdrop-filter: blur(4px);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • [OPTIONAL] You've done a fantastic job with your mobile menu! I have a small optional suggestion that could enhance its professional appearance. Would you consider adding a position fixed to the overlay when the menu is opened? Currently, users are able to scroll down while the mobile menu remains at the top, and implementing this change would address that issue.

    scrnli_6_28_2023_7-07-14.AM.webm

@the-faizmohammad the-faizmohammad merged commit 2272274 into main Jun 28, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants