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

CRDCDH-133 Updating Program/Study #42

Merged
merged 11 commits into from
Jul 14, 2023
Merged

CRDCDH-133 Updating Program/Study #42

merged 11 commits into from
Jul 14, 2023

Conversation

Alejandro-Vega
Copy link
Collaborator

@Alejandro-Vega Alejandro-Vega commented Jul 12, 2023

Overview
This PR is to add name and abbreviation to the label for program and study Autocomplete inputs.

  • Updated Program/Study logic
  • Added name and abbreviation to input labels
  • Modified autocomplete to accept generic types as values and options
  • Update autocomplete styling to use styled instead
  • Fixed initialValues and applicationData not doing a deep merge causing some nested properties to be misssing
  • Fixed toggleContent being conditionally rendered causing for the input to not appear in the DOM and form parser removing the property from values causing a detected change when navigating through sections
  • Fixed textInput not allowing for empty string values
  • Added key to autocomplete inputs to fix issue cause by study depending on program
  • Updated study names and abbreviations
  • Added Study Description tooltip

…ons, and also updated styling to use styled instead. Updated Section B program/studies to retrieve option labels from value and also check equality based on name and abbrev. Fixed TextInput component not clearing the input to empty.
…fixed switchInput toggleContent being conditionally rendered to instead hiding it to keep it in the DOM. Was causing the input properties to be removed from form parser
@Alejandro-Vega Alejandro-Vega added the 🚧 Do Not Merge This PR is not ready for merging label Jul 12, 2023
@Alejandro-Vega Alejandro-Vega marked this pull request as ready for review July 12, 2023 22:34
@Alejandro-Vega Alejandro-Vega removed the 🚧 Do Not Merge This PR is not ready for merging label Jul 12, 2023
Copy link
Member

@amattu2 amattu2 left a comment

Choose a reason for hiding this comment

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

When saving on Section B (without making any changes), I get an error for the dbGaPPHSNumber field. If I interact with the switch before saving, it doesn't occur. Correction...it occurs only when the switch is off.

image

It doesn't seem to happen on the dev/mvp branch, but if this is unrelated to your changes, we can disregard this issue.

…form to complain input is not focusable when hidden + required
@Alejandro-Vega
Copy link
Collaborator Author

Alejandro-Vega commented Jul 13, 2023

@amattu2 Good catch! It was caused when a hidden input also has a required property, but since the field should be required, then I had to conditionally add that property. I didn't add this field, but was fixing a different problem caused by it which made the unsaved changes dialog come back. I pushed the fixed though, thanks.

@Alejandro-Vega Alejandro-Vega added the 🚧 Do Not Merge This PR is not ready for merging label Jul 13, 2023
@Alejandro-Vega Alejandro-Vega added 🚧 Do Not Merge This PR is not ready for merging and removed 🚧 Do Not Merge This PR is not ready for merging labels Jul 13, 2023
@Alejandro-Vega Alejandro-Vega removed the 🚧 Do Not Merge This PR is not ready for merging label Jul 14, 2023
Copy link
Member

@amattu2 amattu2 left a comment

Choose a reason for hiding this comment

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

Everything LGTM.

Nice work including new test cases also!

@amattu2 amattu2 merged commit 892940e into mvp-1.0.0 Jul 14, 2023
@amattu2 amattu2 deleted the CRDCDH-133 branch July 14, 2023 16:56
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.

2 participants