fix(secondary school): Get rid of custom component for SchoolSelection #17882
Conversation
|
Warning Rate limit exceeded@johannaagma has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (24)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
View your CI Pipeline Execution ↗ for commit 656d4ae.
☁️ Nx Cloud last updated this comment at |
…-for-school-selection
…-for-school-selection
…-selection' of https://github.com/island-is/island.is into fix/secondary-school-remove-custom-component-for-school-selection
| @@ -98,32 +105,26 @@ export type RepeaterItem = { | |||
| emailLabel?: StaticText | |||
| placeholder?: StaticText | |||
| options?: TableRepeaterOptions | |||
| filterOptions?: ( | |||
There was a problem hiding this comment.
Note: I need this to filter out schoolIds selected in other selections, and in the options field we only have access to application (not updated answers) and activeField (updated answers, but only for the index you are working with - I need to look at all other indexes).
Not sure if I should've added this optional filterOptions field, or if I should add index + (updated)answers in TableRepeaterOptions
| @@ -139,6 +143,15 @@ export const SelectController = <Value, IsMulti extends boolean = false>({ | |||
| if (isClearable && newVal === null) { | |||
| clearInputsOnChange([id], setValue) | |||
| } | |||
|
|
|||
| if (setOnChange) { | |||
There was a problem hiding this comment.
If this change makes sense, then maybe I should add to: CheckboxController, DatePickerController, InputController, PhoneInputController and RadioController (same places where clearInputsOnChange was added)
There was a problem hiding this comment.
Lets start with the select an see the need for it
libs/application/templates/secondary-school/src/lib/messages/school.ts
Outdated
Show resolved
Hide resolved
libs/application/templates/secondary-school/src/utils/schoolSelection.ts
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…-for-school-selection
| formTitle: (index) => { | ||
| if (index === 0) return school.firstSelection.subtitle | ||
| else if (index === 1) return school.secondSelection.subtitle | ||
| else if (index === 2) return school.thirdSelection.subtitle | ||
| return '' | ||
| }, |
There was a problem hiding this comment.
This file is 400 lines, so I would suggest to extract as much as possible to util functions so we don't loos the readability of the file.
| options: (application) => { | ||
| const schoolOptions = getValueViaPath<SecondarySchool[]>( | ||
| application.externalData, | ||
| 'schools.data', | ||
| ) | ||
| const isFreshman = checkIsFreshman(application.answers) | ||
|
|
||
| return (schoolOptions || []) | ||
| .filter((x) => | ||
| isFreshman | ||
| ? x.isOpenForAdmissionFreshman | ||
| : x.isOpenForAdmissionGeneral, | ||
| ) | ||
| .map((school) => { | ||
| return { | ||
| label: school.name, | ||
| value: school.id, | ||
| } | ||
| }) | ||
| }, |
| filterOptions: (options, answers, index) => { | ||
| const otherSchoolIds = getOtherSchoolIds(answers, index) | ||
| return options.filter((x) => !otherSchoolIds.includes(x.value)) | ||
| }, |
There was a problem hiding this comment.
I would probably extract this even though it's short...
| setOnChange: (option, application, index) => { | ||
| const schoolOptions = getValueViaPath<SecondarySchool[]>( | ||
| application.externalData, | ||
| 'schools.data', | ||
| ) | ||
| const selectedSchool = schoolOptions?.find( | ||
| (x) => x.id === option.value, | ||
| ) | ||
|
|
||
| return [ | ||
| { | ||
| key: `selection[${index}].school.name`, | ||
| value: selectedSchool?.name, | ||
| }, | ||
| { key: `selection[${index}].requestDormitory`, value: [] }, // clear answer | ||
| ] | ||
| }, | ||
| }, |
| loadOptions: async ( | ||
| { apolloClient, application }, | ||
| lang, | ||
| activeField, | ||
| setValueAtIndex, | ||
| ) => { | ||
| const schoolId = | ||
| activeField && | ||
| getValueViaPath<string>(activeField, 'school.id') | ||
| const secondProgramId = | ||
| activeField && | ||
| getValueViaPath<string>(activeField, 'secondProgram.id') | ||
|
|
||
| if (schoolId) { | ||
| const { data } = await apolloClient.query< | ||
| Query, | ||
| QuerySecondarySchoolProgramsBySchoolIdArgs | ||
| >({ | ||
| query: PROGRAMS_BY_SCHOOLS_ID_QUERY, | ||
| variables: { | ||
| isFreshman: checkIsFreshman(application.answers), | ||
| schoolId, | ||
| }, | ||
| }) | ||
|
|
||
| setValueAtIndex?.( | ||
| 'programOptions', | ||
| data?.secondarySchoolProgramsBySchoolId, | ||
| ) | ||
|
|
||
| setValueAtIndex?.( | ||
| 'secondProgram.include', | ||
| data?.secondarySchoolProgramsBySchoolId.length > 1, | ||
| ) | ||
|
|
||
| const options = | ||
| data?.secondarySchoolProgramsBySchoolId?.map((program) => ({ | ||
| value: program.id, | ||
| label: getTranslatedProgram(lang, { | ||
| nameIs: program.nameIs, | ||
| nameEn: program.nameEn, | ||
| }), | ||
| })) ?? [] | ||
|
|
||
| return options.filter((x) => x.value !== secondProgramId) | ||
| } | ||
|
|
||
| return [] | ||
| }, |
| } else { | ||
| IsClearable = isClearable | ||
| } | ||
|
|
||
| let DefaultValue: any |
There was a problem hiding this comment.
Capitalized is reserved for React components or types
| @@ -188,6 +201,20 @@ export const Item = ({ | |||
| Disabled = disabled | |||
| } | |||
|
|
|||
| let Required: boolean | undefined | |||
There was a problem hiding this comment.
Capitalized is reserved for React components or types
| @@ -213,6 +240,23 @@ export const Item = ({ | |||
| getDefaultValue(item, application, activeValues) | |||
| } | |||
|
|
|||
| let ClearOnChange: string[] | undefined | |||
There was a problem hiding this comment.
Capitalized is reserved for React components or types
| ClearOnChange = clearOnChange | ||
| } | ||
|
|
||
| const SetOnChange = |
There was a problem hiding this comment.
Capitalized is reserved for React components or types
| @@ -139,6 +143,15 @@ export const SelectController = <Value, IsMulti extends boolean = false>({ | |||
| if (isClearable && newVal === null) { | |||
| clearInputsOnChange([id], setValue) | |||
| } | |||
|
|
|||
| if (setOnChange) { | |||
There was a problem hiding this comment.
Lets start with the select an see the need for it
What
The custom component to select school + programs for the application Secondary School is a bit unreadable because it is doing a bunch of stuff and very conditioned, which makes it hard to maintain. Also, that custom component was only approved by Norda with the condition that it would be fixed into regular form field after the 1. feb release).
Had to change the RepeaterItem in Fields a bit, to be able to handle all conditions/updates that are necessary for that form field.
Checklist: