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
bug: Close on overlay click -ModalRecordSelector #3220
bug: Close on overlay click -ModalRecordSelector #3220
Conversation
@seancolsen I created a new PR. Let me know if any changes are required. |
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.
As you can probably tell, the Record Selector is quite complex. One important thing to point out is that it's designed to support "nested" selections, as demonstrated below (on the develop
branch, to give you an idea of how it should work).
2023-09-26_16-08-31.mp4
The need to support nested selections is why the record selector doesn't use the plain modal system and instead uses lower-level primitives.
Here are some issues I have with your changes:
- The drop shadow of the window is no longer visible with your changes.
- The alignment of the nested selector with respect to its parent is incorrect.
- The nested selector does not close when clicking on the overlay, making the behavior inconsistent.
These are not exactly terrible problems. But I'd like these to be fixed before merging.
With the approach you've taken, I'm not sure how easy it will be to fix the above problems.
Another approach I'm considering is this:
- When the root record selector opens, we'd add a
'click'
event listener onwindow
. - For each click, we'd use DOM APIs to see if the click was within the bounds of the top-most record selector window. If so, do nothing. Otherwise, close the top-most record selector window.
- When the root record selector closes, we'd remove that event listener on
window
.
I'm not certain this would be the best way to solve this problem, but it's one thing I thought of.
@seancolsen I tried your approach to address this issue, the code is now working for nested selectors. Could you take a look at it? |
Thanks @FidalMathew. I'll review this, but it may take me a couple weeks to get to it. |
@seancolsen when would it be possible for you to check out the PR? |
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.
@FidalMathew thanks for this! Your fix worked great.
From a code-cleanliness perspective, I didn't like the newModalOpen
variable you added though. It wasn't clear to me why it was there. When I removed it, I realized its purpose — to prevent the event listener from firing as soon as the component mounts, and thus to avoid closing the modal immediately after it opens.
I added 4ca3ca3 with a different approach that I hope you will agree is cleaner. Instead of on:click
, we use on:click|capture
. This allows us to remove the newModalOpen
variable.
Aside from that point, though everything else in this PR seems good, so I'm merging it.
1307b13
Thank you @seancolsen for taking the time to review the PR 😄. |
Fixes #3207
This PR fixes the on-click overlay issue for ModalRecordSelector. The issue is resolved using appropriate z-index values for components.
Screenshots
Checklist
Update index.md
).develop
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin