[DUOS-1964][risk=no] WIP Add DataAccessGovernance section to Data Submission Form #1747
Conversation
connorlbark
changed the title
| }; | ||
|
|
||
| const secondaryGroupHtml = () => { | ||
| const fields = findAllSetFields(consentGroup, secondaryConsentFields); |
There was a problem hiding this comment.
Since teh file is kind of long, I'd prefer keeping related variables next to each other! Can we put const secondaryConsentFields declaration above this? (or we can hard code it into the func parameter, since it's not used anywhere else)
| } = props; | ||
|
|
||
| const primaryGroupHtml = () => { | ||
| const field = findFirstSetField(consentGroup, primaryConsentFields); |
There was a problem hiding this comment.
Can we define primaryConsentFields here too?
| return setFields; | ||
| }; | ||
|
|
||
| const primaryConsentFields = [ |
There was a problem hiding this comment.
Would it be worth converting this into like key:value?
| const primaryConsentFields = [ | |
| const primaryConsentFields = { | |
| generalResearchUse: 'General Research Use', | |
| ... | |
| } |
and then when the array value is required, we can keys(primaryConsentFields) to get the array?
| { value: 'Not Determined', label: 'Not Determined' } | ||
| ]; | ||
|
|
||
| // const dataLocationOptions = ["AnVIL Workspace", "Terra Workspace", "TDR Location", "Not Determined"] |
There was a problem hiding this comment.
Is there a reason the array didnt work? The options looks pretty similar!
| } = props; | ||
|
|
||
| const [consentGroup, setConsentGroup] = useState({ | ||
| ...{ |
There was a problem hiding this comment.
can we move this variable out with a name to describe it? like defaultConsentGroupValues?
| id: idx+'_consentGroupForm' | ||
| }, [ | ||
|
|
||
| div({ |
There was a problem hiding this comment.
Are these divs necessary? Can their attributes be put on ConsentGroupSummary?
| // name | ||
| div({ className: 'form-group' }, [ | ||
| label({ | ||
| id: idx+'_consent_group_name_label', |
There was a problem hiding this comment.
nitpick:
| id: idx+'_consent_group_name_label', | |
| id: `${idx}_consent_group_name_label`, |
| className: 'control-label', | ||
| style: DataSubmitterStyles.header, | ||
| }, ['Consent Group Name*']), | ||
| div({ className: '' }, [ |
There was a problem hiding this comment.
nit:
| div({ className: '' }, [ | |
| div([ |
| input({ | ||
| type: 'text', | ||
| style: DataSubmitterStyles.textInput, | ||
| name: idx+'_consent_group_name', |
There was a problem hiding this comment.
| name: idx+'_consent_group_name', | |
| name: `${idx}_consent_group_name`, |
| div({ className: '' }, [ | ||
| input({ | ||
| type: 'text', | ||
| style: DataSubmitterStyles.textInput, |
There was a problem hiding this comment.
Whenever we have these, I always think it's just better to use css, but c'est la vie! :P
If you wanted to migrate these over to using the FormField objects, I dont know how much of these comments are valuable / useful
|
@lu-c thank you for all the super helpful feedback!! i will look over this later today & incorporate it all. I agree on all your major points (esp. about preferring CSS). My plan is to migrate all of this over to FormField objects once your PR is merged in, which will I think clean up A LOT of what's not so great in my PR :) |
connorlbark
changed the title
Co-authored-by: Gregory Rushton <rushtong@users.noreply.github.com>
Co-authored-by: Gregory Rushton <rushtong@users.noreply.github.com>
Addresses
https://broadworkbench.atlassian.net/browse/DUOS-1964
Have you read Terra's Contributing Guide lately? If not, do that first.