Skip to content

Conversation

@danoswaltCL
Copy link
Collaborator

TL;DR: This change simplifies logic around handling common modal events. It's a refactor of FF stuff but necessary to not repeat the same funny business with segments. It also provides a "Common Modal Event" service that could be used for other similar situations where building out ngrx infrastructure for temporary values feels like a bit much.

Explanation: We basically have 2 modal close patterns in the newer UI modals:

  • Simple Confirm / Delete Modals = we close immediately on submit click, the modal is done whether it is successful or not.
  • Complex Form Modals = we want to wait to close only on success, because we want the user to potentially be able to correct an error in the form. This is a little trickier because success is captured in "effects", and we don't necessarily want all of the actions/selectors/reducer/model updates just to store and manage a transient value that is only needed once.

In order to avoid that, we came up with 3 very different and regrettably convoluted patterns to handle http success callbacks and modal event triggers without storing additional state:

  • Selectors that look for the addition / removal / or edits to an entity to indirectly infer success.
  • Close-modal interceptor configured to close modals when 200 ok returned from certain endpoints.
  • Some calls like for import validation are completely bypassing ngrx altogether to be able to handle http calls directly, which is a whole different can-o-worms that I gave up fighting about, now it's just like that.

This change removes the first 2 above in favor of a simple service that can be called from effects. This way seems much more direct and easy to understand. Technically it still violates ngrx patterns because it causes a side-effect that is unmanaged by ngrx, but I think this is fine.

@danoswaltCL danoswaltCL requested review from bcb37, kawcl and zackcl April 7, 2025 22:01
@zackcl
Copy link
Collaborator

zackcl commented Apr 8, 2025

Do you mean #2320 should go first?

@danoswaltCL
Copy link
Collaborator Author

@zackcl sry i left that comment on wrong PR, this one should go first.

@danoswaltCL danoswaltCL merged commit 444bfb9 into dev Apr 8, 2025
14 of 15 checks passed
@danoswaltCL danoswaltCL deleted the feature/refactor-to-remove-close-modal-interceptor branch April 8, 2025 17:39
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.

2 participants