Skip to content
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

Merged
merged 7 commits into from
Dec 14, 2021
Merged

Show upload status in workspace detail view #210

merged 7 commits into from
Dec 14, 2021

Conversation

JackWilb
Copy link
Member

@JackWilb JackWilb commented Dec 10, 2021

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:

  1. Converts the workspace detail component to composition API (there's one small wrinkle with vue.router that I had to work around. See here) (See code review for updated approach)
  2. Gets all uploads from the API using a new route added in multinetjs
  3. Adds an info alert to the workspace detail view when there are uploads that are not finished

Provide pictures/videos of the behavior before and after these changes (optional)

With upload in progress:
image

Without upload in progress (same as before):
image

Are there any additional TODOs before this PR is ready to go?

TODOs:

@netlify
Copy link

netlify bot commented Dec 10, 2021

✔️ 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

@JackWilb JackWilb marked this pull request as ready for review December 10, 2021 22:00
Copy link
Member

@jjnesbitt jjnesbitt left a 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

Comment on lines 182 to 183
setup(props) {
// const router = useRouter();
Copy link
Member

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

Suggested change
setup(props) {
// const router = useRouter();
setup(props, ctx) {
const router = ctx.root.$router;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See c141af6

Comment on lines 149 to 152

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See c141af6

Comment on lines 250 to 251
// eslint-disable-next-line no-return-assign
watch(localWorkspace, () => requestError.value = null);
Copy link
Member

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

Suggested change
// eslint-disable-next-line no-return-assign
watch(localWorkspace, () => requestError.value = null);
watch(localWorkspace, () => { requestError.value = null; });

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See c141af6

@JackWilb JackWilb requested a review from jjnesbitt December 13, 2021 17:32
@JackWilb
Copy link
Member Author

Thanks for the suggestions, @AlmightyYakob. That clears up the code quite a bit

@JackWilb JackWilb merged commit 521773a into main Dec 14, 2021
@JackWilb JackWilb deleted the upload-status branch December 14, 2021 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show upload status in UI
2 participants