-
Notifications
You must be signed in to change notification settings - Fork 12
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
adding license field to the create dandiset page #839
Conversation
@dchiquito @mvandenburgh - I know I'm not doing this properly, I hardcoded the licenses we are using. I found license types in schema.js file, but not sure what is the best way to extract the information from there.. |
we should discuss this a bit more in the context of not requiring license for embargoed dandisets. see: dandi/dandi-schema#115 |
Types are only used for type-checking and are evaluated at compile time and then discarded, so unfortunately there's no way to access the license types in
|
@mvandenburgh - thank you, will try! |
@djarecka, I converted this to a draft PR since you are still working on it. |
❌ Deploy Preview for gui-staging-dandiarchive-org failed. 🔨 Explore the source changes: 4497b4e 🔍 Inspect the deploy log: https://app.netlify.com/sites/gui-staging-dandiarchive-org/deploys/61fd883f36c872000828f173 |
@@ -75,6 +76,17 @@ | |||
:rules="awardNumberRules" | |||
/> | |||
</div> | |||
<div v-if="embargoed===false"> | |||
<v-combobox |
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.
What are your thoughts on using a v-select
instead of v-combobox
here? IMO the extra autocomplete features v-combobox
provides just confuse things, since there's only two license types to choose from. Also, fwiw the meditor uses a v-select
for licenses.
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.
sorry, I missed the comments.... I'm not sure if I had any specific thoughts, will try to use v-select
@mvandenburgh - I came back to this old PR, I merged the current master, but have some errors, that I don't fully understand. Could you please help me with it |
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.
@djarecka See below suggestions, they should fix the type-checking errors. Let me know if you have any questions.
@@ -117,6 +134,7 @@ export default defineComponent({ | |||
setup(props, ctx) { | |||
const name = ref(''); | |||
const description = ref(''); | |||
const license = ref(''); |
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.
const license = ref(''); | |
const license = ref<License>(); |
This is what is causing the type-check failures. license
is implicitly defined as type Ref<string>
here, but the schema dictates that it's an array of string literals, which is encapsulated in the License
type.
Making this change will make license
instead have a type of Ref<License | undefined>
, which works as long as you also update saveDisabled
to return true if the dandiset is not embargoed and has an undefined license (which we want to do anyways), i.e. add || (!embargoed.value && !license.value)
to it (github code review unfortunately won't let me make that a formal suggestion b/c this PR doesn't modify that code yet).
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.
did you have in mind ref<LicenseType>()
?
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.
No, ref<License>()
(you would also have to add that to the import statement). LicenseType
refers to a string, while License
is an array of LicenseTypes
that is populated by the v-select
(see schema.ts
for those type definitions)
Co-authored-by: Mike VanDenburgh <37340715+mvandenburgh@users.noreply.github.com>
Co-authored-by: Mike VanDenburgh <37340715+mvandenburgh@users.noreply.github.com>
Co-authored-by: Mike VanDenburgh <37340715+mvandenburgh@users.noreply.github.com>
Co-authored-by: Mike VanDenburgh <37340715+mvandenburgh@users.noreply.github.com>
Co-authored-by: Mike VanDenburgh <37340715+mvandenburgh@users.noreply.github.com>
@mvandenburgh - could you help me with the error, don't understand it |
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.
@djarecka I updated the tests as we discussed - this PR looks good to me after removing the redundant code I mentioned below.
Co-authored-by: Mike VanDenburgh <37340715+mvandenburgh@users.noreply.github.com>
🚀 PR was released in |
Adding a new field -
license
to the create dandiset page as a mandatory field.closes #747
this PR requires changes in Dandischema, this is WIP