Use FormLink in DatasetOverview #1096
Conversation
| type: Array, | ||
| required: true | ||
| }, | ||
| projectId: { |
There was a problem hiding this comment.
Now that projectId is added to each form, we don't need to pass it in as a prop. I went ahead and removed that prop, but then there was an issue in the tests of the component. The component now expects the forms to have a projectId property, but in the tests of the component, they didn't. The easiest way to solve that was to specify a dataset for requestData. Doing so automatically transforms the dataset object, adding projectId. Once I was passing the dataset to the test, I felt like it didn't make sense to also pass properties and sourceForms as props, since those are accessible from the dataset.
| @@ -21,7 +21,9 @@ except according to the terms contained in the LICENSE file. | |||
| <expandable-row v-for="(form) in propertiesByForm" :key="form.xmlFormId"> | |||
| <template #title> | |||
| <div class="form-name"> | |||
| <router-link :to="publishedFormPath(projectId, form.xmlFormId)" v-tooltip.text>{{ form.name }}</router-link> | |||
| <form-link :form="form" | |||
| :to="publishedFormPath(form.projectId, form.xmlFormId)" | |||
There was a problem hiding this comment.
We need to specify :to in this case. FormLink defaults to primaryFormPath(), but primaryFormPath() doesn't work super well in this case. That's because it expects the form object to have a publishedAt property.
There was a problem hiding this comment.
Just as projectId is set, we could also set publishedAt to something not null, but this is also fine.
There was a problem hiding this comment.
I agree that that would have been a reasonable approach as well. 👍
| }, | ||
| propertiesByForm() { | ||
| const formMap = new Map(this.sourceForms.map(f => ([f.xmlFormId, { ...f, projectId: this.projectId, properties: [] }]))); |
There was a problem hiding this comment.
We were adding projectId to the forms in formMap, but it actually didn't seem like we ever accessed projectId from formMap. We were used the projectId prop directly instead. Either way, forms will now have a projectId property automatically.
| @@ -1,33 +1,18 @@ | |||
| /* | |||
| Copyright 2023 ODK Central Developers | |||
There was a problem hiding this comment.
We don't need a file header in test files.
| @@ -21,7 +21,9 @@ except according to the terms contained in the LICENSE file. | |||
| <expandable-row v-for="(form) in propertiesByForm" :key="form.xmlFormId"> | |||
| <template #title> | |||
| <div class="form-name"> | |||
| <router-link :to="publishedFormPath(projectId, form.xmlFormId)" v-tooltip.text>{{ form.name }}</router-link> | |||
| <form-link :form="form" | |||
| :to="publishedFormPath(form.projectId, form.xmlFormId)" | |||
There was a problem hiding this comment.
Just as projectId is set, we could also set publishedAt to something not null, but this is also fine.
This PR is in response to getodk/central#670 (comment). It adds hover cards for the forms shown on the entity list overview.
What has been done to verify that this works as intended?
I tried it out locally. I also updated tests.
Why is this the best possible solution? Were any other approaches considered?
FormLinkexpects the form passed to it to have aprojectIdproperty. However, the forms listed in the entity list don't have aprojectId. I think they just have anxmlFormIdandname. I ended up thinking that the easiest thing was to add aprojectIdto these forms when the response for the entity list is received. That way, components don't have to do extra legwork and can just pass each form toFormLinkas-is.I've also added some explanatory comments below.
Before submitting this PR, please make sure you have:
npm run testandnpm run lintand confirmed all checks still pass OR confirm CircleCI build passes