94 mitgliedsantrag #585
94 mitgliedsantrag #585
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
| import * as yup from 'yup'; | ||
| import _ from 'lodash'; | ||
|
|
||
| export function createSchema(validationData, membership_application) { |
There was a problem hiding this comment.
I recommend to rename the parameters as the names suggest there might be user data passed, which is not the case.
I think for instance the following would be better
validateionData -> validationErrMsg
membership_application -> membershipFormDefinition
Finding a very good name for the latter one seems a little harder as it is basically already some sort of schema and its a little harder to not pick confusing naming when this is passed to a createSchema function. But anything that makes it clear that this is not user data is good.
For testing the createSchema function is also good. As the schema can easily be created for tests. The focus here should be to tests the composition of the different yup elements, and not to test yup itself. For instance I would assume that the yup email validator works, but the final schema that we define might easily have a mixup where a mandatory field ends up as optional. When testing this one could also see whether you has a "fail fast" feature which one could switch off for testing. This would then allow a test to for instance submit an empty json and get validation errors for all the required fields in a single test.
| @@ -0,0 +1,34 @@ | |||
| import jsPDF from 'jspdf'; | |||
|
|
|||
| export function generatePDF(values, membership_application) { | |||
There was a problem hiding this comment.
Again a bit confusing naming with membership_application, which only contains schema information.
| const data = await directus_fetch(MembershipQuery); | ||
|
|
||
| return { | ||
| membership_application: data.membership_application, |
There was a problem hiding this comment.
Again confusing naming, which means that the naming in directus is maybe not optimal as well, there might be additional considerations for the directus naming, but when creating the object here the name should be changed to something more clear from the website perspective.
| }); | ||
|
|
||
| export let data; | ||
| $: membership_application = data.membership_application.data.fields; |
There was a problem hiding this comment.
There is a bit of a naming discrepancy here in directus and for the website code. In directus data has fields which themselves again have fields. The Website calls this groups and fields respectively. It might be a good idea to align it towards to terminology of the website, which I think is clearer.
|
|
||
| export let data; | ||
| $: membership_application = data.membership_application.data.fields; | ||
| $: validationData = data.membership_application.data.validation; |
There was a problem hiding this comment.
As mentioned earlier, I think naming that makes clear that these are error messages would be better.
| } | ||
| }, | ||
| extend: reporter, | ||
| onSubmit: async (values) => { |
There was a problem hiding this comment.
Same as for the validation. Creating this callback should be isolated such that it can be tested in isolation.
| <p class="pb-3">{group.description}</p> | ||
| </div> | ||
| <div class="gap-5 lg:grid lg:grid-cols-2"> | ||
| {#each group.fields as field} |
There was a problem hiding this comment.
One could consider making a FormField component that isolates a lot of this logic here, that way one could more easily test that all the different types of fields work in isolation. But since errors here are likely visual I think its less of a priority compared to carefully testing callbacks that may fail in the background.
| > | ||
| {#if field.type === 'checkbox'} | ||
| <label for={field.name} class="mb-2 mt-4 font-semibold" | ||
| >{field.mandatory === true ? '* ' : ''}{field.label}</label |
There was a problem hiding this comment.
| >{field.mandatory === true ? '* ' : ''}{field.label}</label | |
| >{field.mandatory ? '* ' : ''}{field.label}</label |
| /> | ||
| {:else if field.type === 'radio_group'} | ||
| <h3 class="mb-2 mt-4 font-semibold"> | ||
| {field.mandatory === true ? '* ' : ''}{field.label} |
There was a problem hiding this comment.
| {field.mandatory === true ? '* ' : ''}{field.label} | |
| {field.mandatory ? '* ' : ''}{field.label} |
| {/each} | ||
| {:else} | ||
| <label for={field.name} class="mb-2 mt-4 font-semibold" | ||
| >{field.mandatory === true ? '* ' : ''}{field.label}</label |
There was a problem hiding this comment.
| >{field.mandatory === true ? '* ' : ''}{field.label}</label | |
| >{field.mandatory ? '* ' : ''}{field.label}</label |
|
Thank you so much @KonradUdoHannes !! |
|
An internal decision was made to use an external provider for membership applications |
No description provided.