-
Notifications
You must be signed in to change notification settings - Fork 39
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
Close calendar event popover when clicking outside it #337
Conversation
Also closes event popover on ESC key press.
dce0410
to
ebb9efc
Compare
span.onclick = function() { | ||
modal.style.display = "none"; | ||
} | ||
// When the user clicks anywhere outside of the modal, close it | ||
modal.onclick = function(event) { | ||
if (event.target.id == "myModal") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a stupid question (definitely a naive one): Should there be a !
or a not
somewhere here to trigger when the click is outside the modal target area?
(I've not tested this because on CI the calendar events don't show)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah my intuition is the same as you @psobolewskiPhD, very good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually no, MyModal
is the shadow that is overlayed in front of the whole page while the popover itself is the event
, so this is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, amazing! Maybe change the name to "mySneakyBackgroundHidingModalNotTheModalYouReThinking"??? 😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@melissawm so to clarify, this will only work once the new theme is released and the constraints are upgraded, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here is correct. But I prefer to change the name of myModal
to something more readable. For example, eventDetailBackground
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Czaki that would require a theme change, right? I suggest we merge this and then make the parallel changes after releasing 0.4.19...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused: isn't the ID of the background set by the theme? Clearly this is not an arbitrary string, this is a string matching an element of the DOM? Where is the ID of the modal coming from if not the theme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the expression if (event.target.id == "myModal")
# References and relevant issues #337 (comment) # Description Rename to increase code readability
# References and relevant issues napari/docs#337 (comment) # Description Rename to increase code readability
References and relevant issues
Closes napari/napari-sphinx-theme#138, together with napari/napari-sphinx-theme#147
Description
Clicking outside of the calendar event modal or pressing the ESC key closes the modal.