issue-5063 changes #5083
issue-5063 changes #5083
Conversation
There was a problem hiding this comment.
Hi @yeshwanth235, nice work here, and all works as expected. Leaving few comments for code.
|
|
||
| <KModal | ||
| v-if="showExportDataNotice" | ||
| :appendToOverlay="true" |
There was a problem hiding this comment.
Is this needed? Were there any issues with stacking when it weren't present?
| :submitText="$tr('exportDataBtn')" | ||
| @submit="showExportDataNotice = false" | ||
| :title="$tr('exportStartedHeader')"> | ||
| <template> {{ $tr('exportAccountDataModalMessage') }} </template> |
There was a problem hiding this comment.
I think that wrapping text in <template> shouldn't be needed
| @@ -204,6 +208,9 @@ | |||
| channelLink(id) { | |||
| return window.Urls.channel(id); | |||
| }, | |||
| closeExportDataNotice() { | |||
There was a problem hiding this comment.
This method is not used from anywhere. Can be removed.
| exportData.mockImplementation(() => Promise.resolve()); | ||
| await wrapper.find('[data-test="export-link"]').trigger('click'); | ||
| await wrapper.find('.actions .button').trigger('click'); | ||
| expect(wrapper.vm.showExportDataNotice).toBe(false); |
There was a problem hiding this comment.
Testing whether a modal is displayed via checking on showExportDataNotice value would be a fragile approach. Imagine a situation when we'd re-implement or removed internal data such as showExportDataNotice. Then this test could pass even if the form weren't visible, or fail if it was. For that reason, it's a good practice to write test as close as possible to actual user experience. Here, we'd want to do something similar to this test - check for KModal presence and make sure that the right modal is displayed via checking its text. Before we merge, let's resolve this.
I realize this is a pattern that is present in other tests around this dialog. If you'd like to fix them too, that'd be welcome, but it's not required as it was pre-existing.
There was a problem hiding this comment.
Thanks @yeshwanth235, just mention me in the PR if you had any questions or as soon as all feedback is resolved.
There was a problem hiding this comment.
Thanks for following the guidance and updating these!
Summary
Changed Alert Component to KModel
References
#5063
Reviewer guidance
Export data from setting->account
click Export data button