Replies: 6 comments
-
These are all good points. However, I'd say that the modalApi state is suppose to be invisible to the Modals, and the only way Modals should be interacting with this state is through the public methods, I do agree that perhaps the name |
Beta Was this translation helpful? Give feedback.
-
Also, why is |
Beta Was this translation helpful? Give feedback.
-
@pretzelhammer I think it's because in a lot of cases, we have API logic that needs to resolve before actually closing the modal and showing the full page experience. This makes it so buyers can see the loading state inside the modal before those requests have resolved. |
Beta Was this translation helpful? Give feedback.
-
@mattmewton yeah, I understand the need for, and agree with, there being <!-- setting beforeClose hook in the template -->
<template>
<m-modal :before-close="myCloseFn">
<!-- etc -->
</m-modal>
</template> and // setting beforeClose hook in JS
this.modalApi.open(() => <MyModal />, { beforeClose: myCloseFn }); whereas there's only 1 way in Dialogs, which is the JS way. I'm not sure whether it's better to add the prop to Dialogs or to remove it from Modals. From an API design standpoint it's usually better to reduce the public API surface where possible, as it makes it easier to learn for users and makes it easier to maintain for devs. Right now there's a subtle edge case where different beforeClose hooks can be defined in different places for the same modal and one will just silently override the other, but this override behavior is not documented anywhere and even if it was it's the sort of subtlety that users of maker modals are likely to overlook soooo I don't feel super great about it... and would kinda prefer there to only be 1 way to set a beforeClose hook on a specific modal... but making this change isn't urgent. I suppose another alternative would be to make the inner beforeClose hook not override the outer beforeClose hook but set to a different name internally, and then both are resolved, with the inner one executing first and then the outer one. That would probably work 🤔 |
Beta Was this translation helpful? Give feedback.
-
@pretzelhammer yeah, I agree with your points. I do see the prop being used a decent number times in the Website repo, so it would be hard to outright remove it. The best course of action might be to release a fix with the inner hook set to a different name internally, as you suggested. Then in a future, breaking version, we could remove the prop altogether and stick with the JS hook, which makes more sense semantically. |
Beta Was this translation helpful? Give feedback.
-
Okay, i dug around modal internals and fixed all the bugs i could find, i also renamed |
Beta Was this translation helpful? Give feedback.
-
Continuing discussion from this PR: #380 (comment). We ultimately removed the modal changes to move forward with our project.
There's room for some confusion when working with modal layers in Maker. This is mainly due to the fact that the provided
modalApi
state is actually for the layer one level above that one would expect.For example, let's take a simple setup where we have a component open a modal:
When the component is mounted, you'd see the state of the modal (with the defined
beforeCloseHook
option).Let's now take a look at the modal we opened:
Once this modal component is mounted, you can see from the printed state that the hook is no longer visible.
This is because in the internal
ModalLayer
code, opening a modal creates a newmodalApi
object and injects that for the newly created modal component.This creates a disconnect, where the
modalApi
object that a modal interacts with is not actually for the layer that it exists in.We currently get around this by creating a
currentLayer
provider for theModalLayer
component, but it is not easily exposed via themodalApi
provider.I don't have a solid proposal to clean this up. I do think that we should shift the
modalApi
to contain the current layers state instead of the state above it. Or at the very least expose it in an ergonomic way.cc @landondurnan @pretzelhammer
Beta Was this translation helpful? Give feedback.
All reactions