refactor!: popup-nav-dismiss

[chrismclarke]

PR Checklist

• [ ] PR title descriptive (can be used in release notes)

Description

Possible alternate implementation for behaviour in #2134

Breaking changes

Added by @jfmcquade: Also renames the parameter passed to the go_to action: dismiss_on_return -> dismiss_pop_up. This more accurately describes the behaviour of the feature. This is a breaking change that will require changes to the templates that implement this feature, which appear to be in_person_unreported and virtual_unreported. (Alternatively we could support both syntaxes to avoid deprecation, but it should be easy to make the changes at this stage and avoid including the deprecated syntax in the code).

Git Issues

Closes #

Screenshots/Videos

Updated debug_pop_up_return template:

Updated behaviour showing preservation of query params (previously all query params would have been removed on navigation away from to the pop-up):

https://github.com/IDEMSInternational/parenting-app-ui/assets/64838927/11d1a1d1-3946-43a5-b4b3-e4a5c45044c5

[chrismclarke]

Thanks, @chrismclarke, I like your refactor. I've updated the PR description and also changed the name of the passed parameter dismiss_on_return -> dismiss_pop_up, which was agreed to be a more accurate description of the functionality.

I think this is ready for a functional test. And I think worth merging in anticipation of any query params issues rather than waiting to see if any emerge.

Thanks for the improvements and identifying the breaking change - all makes sense to me. In hindsight, do we already have a click action for popup_dismiss? If so that could have been the solution from the start (i.e. add a popup dismiss action before the nav one), but no harm giving authors multiple options I suppose...

[jfmcquade]

In hindsight, do we already have a click action for popup_dismiss? If so that could have been the solution from the start (i.e. add a popup dismiss action before the nav one), but no harm giving authors multiple options I suppose...

I'm not aware we have such an action, but this would make lot of sense to me.

It appears there was a close_pop_up action ID listed in the code, but no logic to actually handle the functionality, so I've removed its reference.

I agree that a standalone pop_up_dismiss action might have been a simpler approach... Although I don't think making use of the TemplateNavService.dismissPopup method without modifications would work, I think I did try that and the popup wasn't dismissed when navigating back to the page.

In any case, I think this PR should be ready to merge now.