Skip to content
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

Allow proposed modal to dismiss a currently presented modal #168

Closed
wants to merge 3 commits into from

Conversation

olivaresf
Copy link
Member

This is the alternative to #164.

Currently there's no way to jump from one modal flow to another. The app has to manually dismiss the first modal flow, then request a new modal flow by creating a new visit proposal with a modal context.

Adding an "always_dismiss" key allows us to modify the proposal so that the above is now possible. If Modal A is presented, and Modal B is proposed, Modal B can now dismiss Modal A before being presented by setting "always_dismiss" to true.

@olivaresf olivaresf changed the title Allow a modal to dismiss a currently presented modal Allow proposed modal to dismiss a currently presented modal Dec 15, 2023
@jayohms
Copy link
Collaborator

jayohms commented Dec 15, 2023

Is the always_dismiss flag associated with modal A or modal B in the path config?

@olivaresf
Copy link
Member Author

Is the always_dismiss flag associated with modal A or modal B in the path config?

That's a good question 🤔. I'm associating it with modal B... if it caused confusion, maybe a rename is in order. Something like "wants_new_modal_context" or something along those lines?

@joemasilotti
Copy link
Member

What about context: "new_modal"? That way it's not another property and is more obvious that it will fall back to "modal".

@jayohms
Copy link
Collaborator

jayohms commented Dec 15, 2023

Hmmm, interesting. Historically, it's always made sense to associate it with modal A. The thought is: You likely won't know where modal A might take me (especially as features expand), but I know when a new navigation is proposed from modal A, dismiss modal A first.

And yes, good call, let's clarify the language. My vote would be to associate the property with modal A. Is there a reason you don't see that working? Depending on where the rule goes, the naming will need to reflect that.

@olivaresf
Copy link
Member Author

My vote would be to associate the property with modal A. Is there a reason you don't see that working? Depending on where the rule goes, depends on the naming.

I think there's two different, but equally valid, options:

  • Associating it with modal A: This ensures Modal A is a stand-alone modal, meaning its navigation controller will not push any new view controllers.
  • Associating it with modal B: This ensures Modal B is always the root controller in the modal navigation controller.

I see more drawbacks associating it with Modal A. If Modal A ever wants to allow other modal controllers to be pushed into its stack, wouldn't we have to set "always_dismiss" to each pushed controller to preserve that behavior? It's a contrived scenario, but I think it makes it immediately obvious that associating "always_dismiss" with Modal A is very strict behavior.

My preference is still associating it with Modal B, as Modal B already defines its context, presentation, and style. Having it define whether it wants to be the root makes more sense, imo. What do you think?

What about context: "new_modal"? That way it's not another property and is more obvious that it will fall back to "modal".

I don't think "new_modal" is clear enough. The other thing to consider is that this is sort of close to "replace", but not quite. It's more like "requires_new_context"? "new_modal_context"? "clear_navigation_stack"? "recede_before_presenting"?

Or maybe just go with the obvious "dismiss_before_presenting" 😄?

@jayohms
Copy link
Collaborator

jayohms commented Dec 15, 2023

What if we went a different route... and let you specify an optional modal_group with a key value. To maintain a modal controller/stack, each visit proposal would have to match the current modal_group — otherwise start a new one. So, it'd look something like this:

{
  "rules": [
    {
      "patterns": [
        "/new$",
        "/new\\?.+$",
        "/edit$",
        "/edit\\?.+$"
      ],
      "properties": {
        "context": "modal"
      }
    },
     {
      "patterns": [
        "/accounts/*"
      ],
      "properties": {
        "context": "modal",
        "modal_group": "accounts"
      }
    },
  ]
}

This would allow you to always maintain an "account" separate modal stack no matter how or where it was presented. If you're in a /feature/new modal and you go to /accounts/users, we'd enforce a new "account" modal stack. Same behavior if you were in accounts/users and went to feature/new.

@olivaresf
Copy link
Member Author

This would allow you to always maintain an "account" separate modal stack no matter how or where it was presented. If you're in a /feature/new modal and you go to /accounts/users, we'd enforce a new "account" modal stack. Same behavior if you were in accounts/users and went to feature/new.

I'm not sure I follow. What do you think would be the benefit of this instead of assigning "always_dismiss" (or whatever we name it) to "account/users"? I feel "always_dismiss" solves the issue without adding the responsibility of keeping track of the current stack.

@jayohms
Copy link
Collaborator

jayohms commented Dec 15, 2023

It adds a ton of flexibility with a single property. You should be able to solve every situation you can think of with it. It means you can keep a modal stack going for a set of urls (or just one if you prefer), but dismiss/open a new modal once you navigate away from the current modal_group.

With the always_dismiss approach, if you want to keep /accounts/users and /accounts/edit in the same modal stack, but dismiss it whenever you navigate outside of account urls, that's not possible — unless you know exactly what urls you those account screens might navigate to. And even then, let's say from /accounts/projects, you navigate to /projects/edit. You can get to /projects/edit from other places, too. So if you assign /projects/edit with always_dismiss: true that means it will always dismiss any modal context any time you visit /projects/edit even if that's undesirable.

I feel "always_dismiss" solves the issue without adding the responsibility of keeping track of the current stack

There shouldn't really be any work to keep track of the current stack. You just have know the current url of the presented modal and look up its modal_group value in the path config.

@joemasilotti
Copy link
Member

How does Android handle this?

@jayohms
Copy link
Collaborator

jayohms commented Dec 15, 2023

It doesn't, but I'd love to add support too :)

I'm pretty convinced that a simple boolean is not sufficient to handle the situations that might come up. It's worked fine internally for a few rudimentary situations, but at the framework level it needs to be more flexible. With a modal_group you only need to know the url(s) you want in the current modal stack OR the url(s) you want in a new stack. And if you don't want to use the feature, then it works exactly like it does now — you always stay in the same modal stack as long as the urls remain in the modal context.

@joemasilotti
Copy link
Member

OK, cool! Just wanted to make sure we weren't missing anything we could learn from Android.

To be honest, I feel a little lost about the business/product goal of having these modals split out like this. Is it possible to provide a concrete example (ideally from HEY or Basecamp!) of where this configuration would be useful? The abstract examples aren't really clicking with me yet.

@olivaresf
Copy link
Member Author

I've added some more context in our Basecamp discussion, @joemasilotti! For now, I think we should probably discuss Jay's idea there and I can come up with another PR. I'm hopeful we're getting very close to a good solution using Jay's idea!

@olivaresf olivaresf closed this Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants