search

okTurtles / group-income

A decentralized and private (end-to-end encrypted) financial safety net for you and your friends.
https://groupincome.org
GNU Affero General Public License v3.0
329 stars 43 forks source link

#935 modal improvements (part 3) #998

Open sdegutis opened 3 years ago

sdegutis commented 3 years ago

Problem

The modal code is not as clean as it could be. Summarized by this comment. This issue is a follow-up to #935, #946, and #997.

Solution

This comment about hashtags seems like the cleanest and simplest way to put modal-identifying data into the URL. But this isn't possible as long as there is any scenario where there is a modal within a modal. Currently, the only place that happens is in the change-password modal, which is inside the settings modal. If that requirement goes away, we could not only implement this URL scheme, but also could significantly simplify Modal.vue by removing a lot of unnecessary logic.

But assuming that requirement can't go away, the second best idea is described in this comment suggesting a before-leave hook. This idea is not significantly better than the current solution, so I advised leaving it as-is and moving onto another task.

Regardless of either ways of solving that issue, it also makes sense to centralize the logic of navigating to a modal, by using vue-router to push the new URL (either via HTML or JS), instead of also having a secondary OPEN_MODAL SBP function. This would reduce the code even further.

taoeffect commented 3 years ago

Thanks for creating this issue @sdegutis! Just wanted to add: the goal of the PR that closes this is to remove the need for SET_MODAL_QUERIES while still preserving our support for two modals (e.g. see the "Change Password" modal scenario), and preserving the goals of simplification mentioned in #935.