feat: add option to persist modal across navigation changes #2545
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2545 +/- ##
=======================================
Coverage 82.99% 82.99%
=======================================
Files 925 925
Lines 20628 20629 +1
Branches 3253 3254 +1
=======================================
+ Hits 17120 17121 +1
Misses 3388 3388
Partials 120 120 ☔ View full report in Codecov by Sentry. |
| @@ -9,6 +9,7 @@ export interface ModalConfig<TData = unknown> { | |||
| title?: string; | |||
| data?: TData; | |||
| closeOnEscapeKey?: boolean; | |||
| persistOnNavigate?: boolean; | |||
There was a problem hiding this comment.
Can we stick with closeOnNavigate? That way it will be consistent with corresponding popover property
There was a problem hiding this comment.
My original thought as well. The reason I went this direction instead is because we close on navigate in every other case. We still want that as our default behavior.
By going this direction, the original usages can be left alone and furthermore future uses can continue to omit the property to use modals in the default behavior.
Like you, I prefer the consistency of your suggestion, but I think the use cases make this a more maintainable option.
There was a problem hiding this comment.
Thinking more, how would a modal stay persist when we navigate? If we navigate to a different page and if we don't destroy the modal, then that could lead to unintended side effects.
There was a problem hiding this comment.
I think that is why this property was not exposed before. cc @aaron-steinfeld if you have some thoughts on this one
There was a problem hiding this comment.
Yeah I'm not a huge fan. The use case was to change query params rather than the whole modal, and I could see supporting that (and probably by default - because it would be weird to have a query param change close a modal), but we also want to be careful about a modal changing the url in the first place.
On a slightly separate note :
By going this direction, the original usages can be left alone and furthermore future uses can continue to omit the property to use modals in the default behavior.
If we proceed, I would still make it consistent - we can default the value to either true or false, we don't have to (and shouldn't anyway) rely on the falsiness of a missing value.
| if (!modalContainerData.config.persistOnNavigate) { | ||
| popover.closeOnNavigation(); | ||
| } |
There was a problem hiding this comment.
I'm curious. whats the usecase and how would this work in practice? when we show a modal, usually the backdrop is non-interactable. How would a navigation get triggered?
Description
Please include a summary of the change, motivation and context.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.