-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
#2245 - Make non-monetary contribution mandatory in IncomeDetails.vue for non-pledgers #2292
#2245 - Make non-monetary contribution mandatory in IncomeDetails.vue for non-pledgers #2292
Conversation
case 'nonMonetaryReplace': | ||
groupProfile.nonMonetaryContributions = cloneDeep(value) | ||
break |
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.
Adding this feature requires a way to update groupProfile.nonMonetaryContributions
array as a whole instead of element by element. So adding nonMonetaryReplace
here.
pledges: { | ||
[L('At least one non-monetary pledge is required')]: (value) => { | ||
return this.optional || | ||
(value?.length && value.some(entry => entry.value)) | ||
}, |
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 need to validate this component when non-monetary contribution is optional. So implementing a custom validator here.
Test summaryRun details
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud |
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.
Fantastic work @SebinSong!
This is just a comment / question for you before approval.
return { | ||
form: { | ||
pledges: [ | ||
{ id: randomHexString(10), value: '' } |
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.
Is this id
thing necessary? If all it's used for is is the :key
in the v-for
, then can't the position (index
) in the array be used instead?
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.
@taoeffect Can we keep this as is? We have a feature to add/remove an array item here and in this case, having unique id like this can help prevent duplicated keys from being generated accidentally.
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.
yeah, I was was just trying to understand why it was necessary
i18n.is-title-4 Non-monetary pledge | ||
i18n.c-optional(v-if='optional') (optional) |
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.
Note: typically I'd request that these be made two complete sentence using a computed property, however, I see that in the UI they are visually separate elements, so this is OK.
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.
Great work!
closes #2245
Adding below block to
IncomeDetails.vue
@taoeffect
Please let me know if you would like to change the validation copy.