-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add file upload instructions to file browser UI #1342
Conversation
f5b7287
to
90eab19
Compare
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.
If you can fix the typing issue, that would be good to include here. Otherwise, because this is a PR to non-master branch, I'm fine with you merging it pending a bigger review of that branch when it goes to master.
import { useDandisetStore } from '@/stores/dandiset'; | ||
|
||
const store = useDandisetStore(); | ||
const dandisetIdentifier = computed(() => store.dandiset?.dandiset.identifier); |
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.
There's a small typing problem here. dandisetIdentifier
has type string | undefined
but you're assuming it's defined at the point of use in the 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.
Fixed 5c98f19
To avoid the installation instruction versions becoming stale, should we link to https://www.dandiarchive.org/handbook/10_using_dandi/#dandi-python-client instead? |
Good idea, I think that makes more sense. I added that link and a link to the file upload instructions. |
I think it's better to have the instructions listed explicitly, rather than kick the user over to the full instructions. I understand there's a risk of things going out of sync, but also the dandi CLI calling conventions are not likely to change very much. I do think adding a link to the docs alongside the customized command sequence for the given dandiset would be a good idea. |
I was only referring to the second block which specifies py37 and dandi>0.13.0. I agree that the first block is unlikely to change. |
92b2cc5
to
6da6d71
Compare
Updated to only change the text about installing the CLI. |
@waxlamp and @mvandenburgh - given this is a web app, how about we create the text that it reads in some location separate from the web app, and it can pull the info, which we can update async from development of the app. btw, i love this feature. |
If this text were going to be in rapid flux (or if it turns out to be like that) I think we could consider this approach. For a first implementation, I prefer the simplicity of what we have in this PR.
🥳 |
f4cf2ed
to
0934006
Compare
6da6d71
to
274e4f3
Compare
🚀 PR was released in |
Closes #732
Closes #731