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

Update funding source entry text box and description #1345

Merged
merged 5 commits into from
Aug 21, 2024

Conversation

JamesTessmer
Copy link
Collaborator

@mslarae13 @pkalita-lbl
This PR adds some small changes to the funding_source description to point towards and example.
I figure if we also want to make the funding_source box accept multiple values we can add that change here as well.

@mslarae13
Copy link

I think this is the PR we agreed would also
Add ability to add another funding request like how contributors works

Add a "+ Add Funding Source" button that makes another text box pop up

@JamesTessmer
Copy link
Collaborator Author

Yes this is the PR for those changes. I'll update the title to reflect the update to how funding sources will be added.

@JamesTessmer JamesTessmer changed the title Update funding source description Update funding source entry text box and description Aug 16, 2024
@JamesTessmer
Copy link
Collaborator Author

Screenshot 2024-08-20 at 9 22 37 AM Here's an image of what the new `fundingSources` section looks like. @mslarae13 @pkalita-lbl Let me know if there's anything stylistically that doesn't look right. I mostly just lifted the same style from the `Contributors` section.

@pkalita-lbl There are 3 things that need updating (or one two and one that could possibly stay as is):
The first is that to support the multiple values for fundingSources I added a FundingSource class which really only contains a string value. I was having issues getting just the list of strings to work since I was iterating over it and mutating the value in the same iteration. I think this will require some small changes in the runtime PR that's still open but that's ok with me unless there's a better way to collect the multiple values here.

The second issue was that the hint on the v-text field for the fundingSource isn't displaying links correctly and I can't figure out what the issue is. I tried plugging in other descriptions that had links and worked in other sections of the studyForm but those had the same issue despite working fine elsewhere on the page.

The last thing is that I added an addFundingSource function. Initially I couldn't get it to work and actually add the FundingSource to the list so that the text box would pop up, and I found out the problem was that the list holding the FundingSources wasn't ever initialized (I think this was the issue anyway). So in that addFundingSource function I added an if that sets the value of FundingSources in studyForm and everything works as expected after that. I tried looking for where the Contributors list is initialized but couldn't find where that is.

@pkalita-lbl
Copy link
Collaborator

Nice progress! Some things to consider:

  • I think storing fundingSources as { value: string }[] isn't necessary, and it can be simplified to string[]. That will make it more inline with the underlying schema. But, in that case the v-model on the input needs to index into the fundingSources array and not refer to a local iteration variable.

    In other words, not this (simplifying a little):

    <div v-for="fundingSource, i in studyForm.fundingSources">
      <v-text-field v-model="fundingSource" />
    </div>

    But, instead this (simplified):

    <div v-for="_, i in studyForm.fundingSources">
      <v-text-field v-model="studyForm.fundingSources[i]" />
    </div>
  • In studyFormDefault I would type fundingSources as:

    fundingSources: [] as string[] | null,

    That way it agrees with the backend's use of Optional, acknowledging that it could be unset for existing submissions. That will also necessitate doing a null check in addFundingSource:

    if (studyForm.fundingSources === null || studyForm.fundingSources.length === 0) {
  • The message slot is used to render the help as HTML:

    <v-text-field ... >
      <template #message="{ message }">
        <span v-html="message" />
      </template>
    </v-text-field>

    Check out the "Principal Investigator ORCID" input for an example.

  • Just OCD things:
    image

@JamesTessmer
Copy link
Collaborator Author

Perfect thanks Patrick. I'll update it to just be a list of strings and not use the object and make the rest of the changes.
I'm also pretty blind to a lot of design things so those kinds of comments on alignment, margins, etc. are usually exactly what I'm looking for.
Hopefully I can knock out those changes later today/early tomorrow.

Copy link
Collaborator

@pkalita-lbl pkalita-lbl 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! I tested locally and the new inputs behaved the way I expected.

@pkalita-lbl pkalita-lbl merged commit 5ad07cc into main Aug 21, 2024
2 checks passed
@pkalita-lbl pkalita-lbl deleted the update-funding-source-text-box branch August 21, 2024 20:08
@JamesTessmer
Copy link
Collaborator Author

Awesome, I'll test the runtime agin with these changes and make any changes needed in that PR that's still open

@pkalita-lbl
Copy link
Collaborator

@mslarae13 please test out the multiple funding source inputs on https://data-dev.microbiomedata.org as well if you have a minute.

@mslarae13
Copy link

Thanks @pkalita-lbl !
I think funding source is required by nmdc-schema & mongo right? So we should probably make this field required.
But I can make a separate issue for this & we can call this good to roll out to avoid issues / bugs for Monday :)

@mslarae13
Copy link

But it works and LGTM!

@pkalita-lbl
Copy link
Collaborator

I think funding source is required by nmdc-schema & mongo right?

No, it is not required according to nmdc-schema. All of the existing study records in Mongo happen to have it set, but the schema doesn't enforce that.

@mslarae13
Copy link

perfect!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants