This PR makes progress on #676, adding a replacement for the modal mixin. The new modalData() returns a reactive object that is similar to the modal object data properties that we've used in the past. However, a modalData() object has its own show() and hide() methods, meaning that a separate mixin (or composable) isn't needed for those actions.
This PR introduces modalData(), but it doesn't use it everywhere: it doesn't replace all existing uses of the modal mixin. Instead, I replaced the modal mixin in a few different types of components:
FieldKeyList. This component has three modals. The use of modalData() allows us to remove two component methods related to toggling a specific modal (showRevoke() and hideRevoke()).
EntityList. This component has some of our most complex modal logic. For example, you can move between the parallel updates modal and the update modal.
DatasetEntities. This component has an async modal component.
I also introduced modalData() to Composition API components that have modals, but that weren't able to use the modal mixin: DatasetSettings and EntityShow. We're about to add a new modal to EntityShow (the EntityDelete modal), so it will be nice to have modalData() available for use.
What has been done to verify that this works as intended?
Tests continue to pass. I also added unit tests of modalData().
Why is this the best possible solution? Were any other approaches considered?
I considered replacing the mixin with a composable, but I think a reactive object with show() and hide() methods is more ergonomic. We need reactivity, but we don't need anything else that a composable would provide (e.g., lifecycle hooks).
I thought about different ways to structure the modalData() object. I considered storing the props for the modal component in a nested object named data, but I ended up feeling that it was more ergonomic to store them as top-level properties on the modalData() object. That also keeps modalData() objects more similar to how modal object data properties have worked in the past.
Lastly, I thought about having the modalData() object automatically specify a hide event handler to the modal component (by adding a property to the object named onHide). However, different modals have different hide logic. (And I don't see any documented support in Vue for adding multiple event handlers for the same event or for having one event handler override a previous one.) I also think that pattern would be quite magical-seeming. I think it's better that the modalData() object is a simple reactive object with show() and hide() methods.
Before submitting this PR, please make sure you have:
[x] run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
[x] verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced
This PR makes progress on #676, adding a replacement for the
modalmixin. The newmodalData()returns a reactive object that is similar to the modal object data properties that we've used in the past. However, amodalData()object has its ownshow()andhide()methods, meaning that a separate mixin (or composable) isn't needed for those actions.This PR introduces
modalData(), but it doesn't use it everywhere: it doesn't replace all existing uses of themodalmixin. Instead, I replaced themodalmixin in a few different types of components:FieldKeyList. This component has three modals. The use ofmodalData()allows us to remove two component methods related to toggling a specific modal (showRevoke()andhideRevoke()).EntityList. This component has some of our most complex modal logic. For example, you can move between the parallel updates modal and the update modal.DatasetEntities. This component has an async modal component.I also introduced
modalData()to Composition API components that have modals, but that weren't able to use themodalmixin:DatasetSettingsandEntityShow. We're about to add a new modal toEntityShow(theEntityDeletemodal), so it will be nice to havemodalData()available for use.What has been done to verify that this works as intended?
Tests continue to pass. I also added unit tests of
modalData().Why is this the best possible solution? Were any other approaches considered?
I considered replacing the mixin with a composable, but I think a reactive object with
show()andhide()methods is more ergonomic. We need reactivity, but we don't need anything else that a composable would provide (e.g., lifecycle hooks).I thought about different ways to structure the
modalData()object. I considered storing the props for the modal component in a nested object nameddata, but I ended up feeling that it was more ergonomic to store them as top-level properties on themodalData()object. That also keepsmodalData()objects more similar to how modal object data properties have worked in the past.Lastly, I thought about having the
modalData()object automatically specify ahideevent handler to the modal component (by adding a property to the object namedonHide). However, different modals have different hide logic. (And I don't see any documented support in Vue for adding multiple event handlers for the same event or for having one event handler override a previous one.) I also think that pattern would be quite magical-seeming. I think it's better that themodalData()object is a simple reactive object withshow()andhide()methods.Before submitting this PR, please make sure you have:
npm run testandnpm run lintand confirmed all checks still pass OR confirm CircleCI build passes