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

Auto close Dropdown when user clicks outside #671

Merged
merged 5 commits into from
Aug 2, 2023

Conversation

balsa-asanovic
Copy link
Contributor

Problem

As stated in #552 the Dropdown options menu of the page can currently be only closed by clicking the arrow again or by clicking any of the available options.

It would be nice for it to also close when the user clicks anywhere outside of the Dropdown.
This pull request introduces this functionality.

fixes #552

Solution

In the PageOptions component, I've added the useEffect hook which attaches event listener to the document on the component mount.
This event listener gets triggered on any click event through the whole document and if it is outside of the Dropdown it closes the options menu.

Testing

Added unit test which checks if the drop down is closed on outside click.

Screenshots

Dropdown opened
image

Dropdown closed on outside click
image

@coveralls
Copy link

coveralls commented Jul 19, 2023

Coverage Status

coverage: 72.079% (+0.03%) from 72.051% when pulling 6f1c0c2 on balsa-asanovic:auto-close-dropdown into c4422b4 on openSUSE:master.

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

Hi @balsa-asanovic,

First of all, sorry for taking so much time to answer. The PR looks great, and it works as expected. I only have a suggestion: using a <div> instead of an <span>. What do you think?

className="page-options"
isGrouped
/>
<span ref={dropdownRef}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: I prefer a <div> instead of <span>. After all, the Dropdown base element is already a <div> so it looks more consistent to me.

Additionally, the <div> element supports flow content while the <span> is limited to phrasing content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah definitely, <div> is a better choice, sorry for missing this.

I've made the commit to PR addressing this.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem, thanks a lot for addressing it 😃

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

Now it looks even better 😃. We just released version 3 today, so your contribution will be part of the next release.

Just for the record, I fixed a minor problem in the changes file (not your fault, just a side effect of updating the file with the "Version 3" entry this morning).

I am approving and merging this fix. Once more time, thanks a lot!

@imobachgs imobachgs merged commit fa00c19 into agama-project:master Aug 2, 2023
7 checks passed
@balsa-asanovic balsa-asanovic deleted the auto-close-dropdown branch August 2, 2023 22:32
@imobachgs imobachgs mentioned this pull request Sep 26, 2023
dgdavid added a commit that referenced this pull request Oct 3, 2023
Which is a kind of rollback of #671
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.

Automatically close a Dropdown when user clicks outside
3 participants