Modified Modal API to allow displaying multiple modals in a stack.
Used array indexes as keys for rendering array of modals, as they never changes between renders - we only push or pop modals from the stack.
Linear: https://linear.app/comm/issue/ENG-967/allow-displaying-multiple-modals-in-a-stack
Details
- Reviewers
tomek • benschac atul - Commits
- rCOMMe2d045ebf313: [web] Allow displaying multiple modals
Tested the feature by adding temporary an action displaying confirmation modal after clicking on member in MembersModal.
Try displaying all existing modals to confirm, their behavior remain unchanged after introducing modal API changes.
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Not sure if you've done that, but we need to confirm one thing: there's a significant difference between calling setModal and pushModal when calling it multiple times. setModal would render the same modal again but push will add its copy. So please check all the calls to pushModal and confirm that each user action / function call would result in at most one call to the function. If we call it from an effect (even indirectly) it might be an indication that we have some issues.
web/app.react.js | ||
---|---|---|
225–231 ↗ | (On Diff #11222) | It looks like in our case we can use this approach: https://robinpokorny.medium.com/index-as-a-key-is-an-anti-pattern-e0349aece318
We call pushModal and never update what's inside
We would need to introduce some kind of id. It sounds like the context could determine it. Not sure if React allows Symbols as keys, but that would be nice.
That's the case. So overall, I think we should check if there's a way to generate unique ids and use them as keys. Even though our current solution should work well, it's less maintainable as it is possible to violate the conditions in the future. But if we can't find a good solution with other keys, we can keep the idx. |
web/modals/modal-provider.react.js | ||
29 ↗ | (On Diff #11222) | We don't need to destructure - slice doesn't modify the original array |
I checked all the usage of existing setModal, and didn't find any case of calling this function multiple times, or from an effect, so the behavior change you described shouldn't cause any problems.
Overall, looks really good!
A couple of questions inline, mostly about clearModals vs popModal
web/app.react.js | ||
---|---|---|
82 ↗ | (On Diff #11464) | We don't need to have optional node here, right? |
240–246 ↗ | (On Diff #11464) | We usually don't use jsx in components that fetch the data (so in this case all the jsx would be in App). But I also don't think there's anything wrong with doing what you've done - so we can probably keep it. |
web/calendar/entry.react.js | ||
389 ↗ | (On Diff #11464) | I'm not sure but it sounds like all the uses of clearModal should be replaced by popModal unless there's a good reason to use clearModals. What do you think? |
web/chat/thread-menu.react.js | ||
164 ↗ | (On Diff #11464) | popModal? |
web/media/multimedia-modal.react.js | ||
53–61 ↗ | (On Diff #11464) | This is an interesting issue. If a user clicks outside, do we want to close all the modals or only the one from the top? I'd say that we should close only one. |
web/media/multimedia.react.js | ||
83 ↗ | (On Diff #11464) | I guess pushModal is always present |
115 ↗ | (On Diff #11464) | We probably don't need this invariant |
web/modals/modal-provider.react.js | ||
36–38 ↗ | (On Diff #11464) | Is it really necessary to support calling this function with null? If it is, then we can generate uuid in the if (or add if(!newModal) { return; } block) |
web/app.react.js | ||
---|---|---|
82 ↗ | (On Diff #11464) | Yes, thanks for catching it |
web/calendar/entry.react.js | ||
389 ↗ | (On Diff #11464) | Agree. This shouldn't make any difference and with all current clearModal usages, both popModal and clearModals should work exactly the same. The popModal however seems to be a bit closer to currently existing intention, which was to clear one, currently displayed modal. I'll replace clearModal with popModal |
web/media/multimedia-modal.react.js | ||
53–61 ↗ | (On Diff #11464) | Agree. It's probably the best to close only the one on the top |
web/media/multimedia.react.js | ||
83 ↗ | (On Diff #11464) | I suppose it is. I don't know why this condition was placed here, and I think it may be safely removed |
web/modals/modal-provider.react.js | ||
36–38 ↗ | (On Diff #11464) | The old setModal had ?React.Node parameter. I don't think it's necessary, so I'll remove the optional |
Looks ok, but the test plan should be improved. Instead of just testing the new modal, we should also test at least a couple of the existing ones.
web/media/multimedia.react.js | ||
---|---|---|
83 ↗ | (On Diff #11626) | This variable was necessary only because of the if. I think we can just have onClick={this.onClick} in the return |