-
Notifications
You must be signed in to change notification settings - Fork 3
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
Show upload status in workspace detail view #210
Conversation
✔️ Deploy Preview for next-multinet-client ready! 🔨 Explore the source changes: 39882a8 🔍 Inspect the deploy log: https://app.netlify.com/sites/next-multinet-client/deploys/61b91dd5be68a00008b41335 😎 Browse the preview: https://deploy-preview-210--next-multinet-client.netlify.app |
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.
Looks good overall, just a few suggestions
src/views/WorkspaceDetail.vue
Outdated
setup(props) { | ||
// const router = useRouter(); |
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.
This is how I use the router with CompositionAPI on Vue2
setup(props) { | |
// const router = useRouter(); | |
setup(props, ctx) { | |
const router = ctx.root.$router; |
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.
See c141af6
src/views/WorkspaceDetail.vue
Outdated
|
||
// import { useRouter } from 'vue-router'; // Router import vue3 syntax | ||
// eslint-disable-next-line import/no-cycle | ||
import router from '@/router'; // Temporary router syntax that lets us use the old push |
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.
// import { useRouter } from 'vue-router'; // Router import vue3 syntax | |
// eslint-disable-next-line import/no-cycle | |
import router from '@/router'; // Temporary router syntax that lets us use the old push |
If my suggestion around setup
is committed, this is no longer needed.
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.
See c141af6
src/views/WorkspaceDetail.vue
Outdated
// eslint-disable-next-line no-return-assign | ||
watch(localWorkspace, () => requestError.value = null); |
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.
Rather than ignore the rule, I'd just follow it here
// eslint-disable-next-line no-return-assign | |
watch(localWorkspace, () => requestError.value = null); | |
watch(localWorkspace, () => { requestError.value = null; }); |
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.
See c141af6
Thanks for the suggestions, @AlmightyYakob. That clears up the code quite a bit |
Does this PR close any open issues?
Closes #174
Give a longer description of what this PR addresses and why it's needed
This PR does 3 things:
Provide pictures/videos of the behavior before and after these changes (optional)
With upload in progress:
Without upload in progress (same as before):
Are there any additional TODOs before this PR is ready to go?
TODOs: