-
Notifications
You must be signed in to change notification settings - Fork 242
[Remove Vuetify from Studio] Permanently delete resource confirmation dialog #5481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Remove Vuetify from Studio] Permanently delete resource confirmation dialog #5481
Conversation
|
Thanks you @z-ABYa, we will assign a reviewer next week. |
contentcuration/contentcuration/frontend/channelEdit/views/trash/TrashModal.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/channelEdit/views/trash/__tests__/trashModal.spec.js
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/channelEdit/views/trash/__tests__/trashModal.spec.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @z-ABYa! Code changes look great, just a small suggestion on component targeting in the tests updated. We should be good for the merge once this small change is made. Thanks you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also notice that we now conflicts, Could you please rebase to the latest unstable and resolve any conflicts therein? Thank you
|
@z-ABYa Thanks for the changes. LGTM! Could you also look into the merge conflicts as well? |
|
Sure @akolson. Sorry for the late reply, i was at a tech event for the last 2 days. |
| </KModal> | ||
| <MoveModal | ||
| v-else | ||
| v-if="moveModalOpen" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @z-ABYa! Please revert this change to the previous code, it not only breaks the restore functionality, but is also outside the scope of the issue. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @akolson, I have reverted to the previous code and made certain only the required changes are committed this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this in't reverted yet. v-if="moveModalOpen" should be replaced with v-else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @z-ABYa! one additional fix and we should be good for the merge. Thanks
fab996a to
81869de
Compare
| @@ -1,173 +1,170 @@ | |||
| <template> | |||
|
|
|||
| <div> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's revert this. And move the <MoveModal...> code to be a child of the div as previously. That way we keep the pr focused to the confirm dialog only. This will prevent potential regressions arising from moving around the code.
<template>
<div>
<FullscreenModal>
......
</FullscreenModal>
<MoveModal
v-else
.....
/>
</div>
</template>There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure i'll make the required changes based on the structure you have suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @z-ABYa! Thanks for the pr updates. I left additional comments for your recent changes. Please do take a look when you have a moment. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @z-ABYa, great work on the migration.
It looks like the migration was actually completed earlier in this commit. However, your cleanup of the unused CSS is valuable and improves the codebase, so we’ll go ahead and merge this.
Feel free to shift your focus to the other issues assigned to you.
Thanks again for your contribution!
Fixes #5443
Summary
Previous (MessageDialogue)

After (KModal)
