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.

Sign up for GitHub

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

Jump to bottom

adding event listener to close navbar dropdown #464

Merged
merged 4 commits into from
Jul 4, 2023
Merged

adding event listener to close navbar dropdown #464

merged 4 commits into from
Jul 4, 2023

Conversation

s2sharpit
Copy link
Member

@s2sharpit s2sharpit commented Jun 28, 2023 •
edited
Loading

Related Issue

  • Information about the related issue

Closes: #459

Describe the changes you've made

Enhance the profile dropdown functionality to automatically close when the user scrolls or clicks anywhere within the active window, providing a seamless and intuitive user experience.

Type of change

What sort of change have you made:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, local variables)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update.
  • This change requires a documentation update

How Has This Been Tested?

Checklist

  • My code follows the guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented on my code, particularly wherever it was hard to understand.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • Any dependent changes have been merged and published in downstream modules.

Screenshots (if applicable)

Original Updated
original screenshot updated screenshot

Code of Conduct

@vercel
Copy link

vercel bot commented Jun 28, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
webxdao-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 1, 2023 9:38am

thehashmap
thehashmap reviewed Jun 29, 2023
View reviewed changes
Copy link
Contributor

@thehashmap thehashmap left a comment

Choose a reason for hiding this comment

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

It works as mentioned.

But the dropdown does not close if you click the navbar anywhere other than the X button. Was that intended? If yes, then it's ok.
Otherwise, you can update for that too.

@s2sharpit
Copy link
Member Author

It works as mentioned.

But the dropdown does not close if you click the navbar anywhere other than the X button. Was that intended? If yes, then it's ok.
Otherwise, you can update for that too.

Hey, It was not intentional. I have fixed it.

@s2sharpit s2sharpit requested a review from thehashmap June 29, 2023 14:23
@vinzvinci vinzvinci requested review from a team June 30, 2023 18:54
@vinzvinci vinzvinci added the in review This PR is in Review label Jun 30, 2023
Anmol-Baranwal
Anmol-Baranwal reviewed Jul 1, 2023
View reviewed changes
Copy link
Member

@Anmol-Baranwal Anmol-Baranwal left a comment

Choose a reason for hiding this comment

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

It works as mentioned, but there might be a couple of things that you can improve.

  1. In the handleClose function, the event parameter is declared as e: Event. It would be more specific and accurate to specify the actual event type we are expecting.
    e.g.
(e: MouseEvent) => {
  1. In the useEffect hook, the dependency array includes handleClose and toggle. It's generally recommended to only include variables or functions that are directly used inside the effect. Since handleClose is a callback function created with useCallback, its reference won't change unless its dependencies change. Therefore, you can omit it if you want from the dependency array to avoid unnecessary effect re-executions.
    e.g.
}, [toggle]);

@s2sharpit
What do you think on this?

@s2sharpit
Copy link
Member Author

s2sharpit commented Jul 1, 2023 •
edited
Loading

Hey @Anmol-Baranwal
I have optimized the code as per your suggestion. Please review it now.

धन्यवाद 😊

@s2sharpit s2sharpit requested a review from Anmol-Baranwal July 1, 2023 09:39
Anmol-Baranwal
Anmol-Baranwal approved these changes Jul 1, 2023
View reviewed changes
Copy link
Member

@Anmol-Baranwal Anmol-Baranwal left a comment

Choose a reason for hiding this comment

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

🚀 Looks clean to me.

@Anmol-Baranwal
Copy link
Member

@s2sharpit
Hey, I noticed you've been making a lot of contributions! Here's a suggestion to help you become an even better contributor in terms of writing commit messages: check out https://www.conventionalcommits.org/en/v1.0.0/. Following these guidelines can really boost your credibility.

Also, it's best to use English instead of Hindi in your responses since not everyone may understand Hindi. Using a widely accessible language like English keeps things professional.

@s2sharpit
Copy link
Member Author

@s2sharpit Hey, I noticed you've been making a lot of contributions! Here's a suggestion to help you become an even better contributor in terms of writing commit messages: check out https://www.conventionalcommits.org/en/v1.0.0/. Following these guidelines can really boost your credibility.

Also, it's best to use English instead of Hindi in your responses since not everyone may understand Hindi. Using a widely accessible language like English keeps things professional.

Thank you, @Anmol-Baranwal, for your feedback and suggestion! I really appreciate your effort in helping me improve as a contributor. I will definitely check out the Conventional Commits guidelines you shared.

vinzvinci
vinzvinci approved these changes Jul 1, 2023
View reviewed changes
Copy link
Member

@vinzvinci vinzvinci left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@vinzvinci vinzvinci added the gssoc23 Used for GirlScript Summer of Code 2023 label Jul 2, 2023
@s2sharpit
Copy link
Member Author

Hi @vinzvinci @Anmol-Baranwal @thehashmap ,
The pull request has been approved. Could you please take a look and merge it if everything looks good?
Thank you!

@Anmol-Baranwal
Copy link
Member

Hi @vinzvinci @Anmol-Baranwal @thehashmap , The pull request has been approved. Could you please take a look and merge it if everything looks good? Thank you!

We will soon get this done. Kindly wait.

@vinzvinci vinzvinci requested a review from a team July 3, 2023 14:49
@vinzvinci vinzvinci added level2 enhancement of existing features and removed in review This PR is in Review labels Jul 4, 2023
@vinzvinci vinzvinci merged commit 4ca750d into WebXDAO:main Jul 4, 2023
@s2sharpit s2sharpit deleted the navbar branch July 7, 2023 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thehashmap thehashmap thehashmap approved these changes

@Anmol-Baranwal Anmol-Baranwal Anmol-Baranwal approved these changes

@vinzvinci vinzvinci vinzvinci approved these changes

Assignees
No one assigned
Labels
gssoc23 Used for GirlScript Summer of Code 2023 level2 enhancement of existing features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Navbar dropdown does not close when clicking outside or scrolling
4 participants
@s2sharpit @Anmol-Baranwal @vinzvinci @thehashmap