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

Clicking outside closes Quarter and Add Year Popovers #406

Merged
merged 5 commits into from
Jan 29, 2024

Conversation

Awesome-E
Copy link
Member

Description

Uses an OverlayTrigger around the buttons that create a popover, which allows React Bootstrap to handle the toggle events for these menus.

Note: This PR does NOT cover the Edit Year popover, as I believe that should be reworked to not include nested menus. Currently, using an OverlayTrigger will close the Year overlay when you click on the input in the Edit Year form because it is outside of the Year overlay.

We could use a custom handler of some sort to determine when to close the overlay trigger, but that's not any better than the what we had before with the Overlays and their handlers, so I'll hold off on changing this.

image

(Clicking the "name" or "start year" fields causes the Overlay Trigger to close)

Screenshots

image image
Clicking outside of these will close them

Steps to verify/test this change:

  • Verify changes work as expected on staging instance

Final Checks:

  • Verify successful deployment

Issues

Partially fixes issue #167

Copy link
Member

@js0mmer js0mmer left a comment

Choose a reason for hiding this comment

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

The showQuarterMenu and show states can be removed since the overlaytrigger now handles that functionality and they're not doing anything.

The popovers do not close when you click a button within them. Previously clicking add year would close the popover. Clicking clear wouldn't close the popover for a quarter but I think it should. A quick hack for this is adding document.documentElement.click() to the onClick handlers for these buttons.

site/src/pages/RoadmapPage/AddYearPopup.tsx Outdated Show resolved Hide resolved
@js0mmer
Copy link
Member

js0mmer commented Jan 28, 2024

Looks like there are a couple merge conflicts after merging my dark mode PR. The only conflicting changes I made were adding a class name to the add year form and using the current theme to determine the button variant for the quarter popover menus. Make sure to implement those on top of your current changes when merging changes from master and you should be fine.

@Awesome-E
Copy link
Member Author

The popovers do not close when you click a button within them. Previously clicking add year would close the popover. Clicking clear wouldn't close the popover for a quarter but I think it should.

Good catch. I did call the event to close it when you click "delete", but forgot to do it for "clear"

@Awesome-E
Copy link
Member Author

Awesome-E commented Jan 29, 2024

The showQuarterMenu and show states can be removed since the overlaytrigger now handles that functionality and they're not doing anything.

This isn't completely true. I am using the onTrigger to call showQuarterMenu and update show because this is what allows clicking the buttons in the menu to actually close it. OverlayTrigger is handling the clicking outside and putting it into a onTrigger event for us and can keep the state on its own, but we want to also be able to have the state separate so we can control it manually for the case I just described.

Additionally, I would rather set state based on the event that's already being fired instead of doing a hack like manually click root to close it.

so that clicking add year closes the popup
Copy link

Deployed staging instance to https://staging-406.peterportal.org

Copy link
Member

@js0mmer js0mmer left a comment

Choose a reason for hiding this comment

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

🚀

@Awesome-E Awesome-E merged commit a337e23 into master Jan 29, 2024
3 checks passed
@Awesome-E Awesome-E deleted the close-overlays branch January 29, 2024 16:31
@js0mmer js0mmer linked an issue Feb 15, 2024 that may be closed by this pull request
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.

Close AddYear Popup when click away
2 participants