Conversation
|
Looks like you did not link an issue to this PR. If this PR completes a task, consider linking it. |
michalges
force-pushed
the
feat/group-inputs
branch
from
3abef5d to
7f73a4c
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces support for grouping related form inputs in the Abstract Resource Form (ARF) and updates resource metadata to use the new grouping for several resources (notably map-related ones), alongside updating external digital guide mode options.
Changes:
- Add
groupInputsto ARF input definitions and render grouped sections recursively inArfBody. - Refactor resource metadata to group address/coordinate (and some guide-related) inputs using
groupInputs. - Update
ExternalDigitalGuideModeenum values and corresponding UI labels.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/features/resources/enums.ts | Updates ExternalDigitalGuideMode enum values. |
| src/features/resources/data/resource-metadata.ts | Refactors multiple resource input definitions to use grouped inputs; adds reusable address/coordinate group configs; updates labels for guide mode. |
| src/features/abstract-resource-form/types/inputs.ts | Adds groupInputs to the ARF input type model. |
| src/features/abstract-resource-form/components/arf-input-set.tsx | Adjusts input mapping logic with additional type assertions to accommodate new input union shapes. |
| src/features/abstract-resource-form/components/arf-controller.tsx | Moves the scroll/container wrapper from ArfBody into the controller layout. |
| src/features/abstract-resource-form/components/arf-body.tsx | Implements grouped input rendering via recursive ArfBody calls and updates layout rules for grouped sections. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const mapped = typedEntries(inputs as Record<string, unknown>).map( | ||
| ([key, options]) => { | ||
| if (options == null) { | ||
| return null; | ||
| } | ||
| return mapper([ | ||
| key as keyof NonNullable<Y>, | ||
| options as NonNullable<ValueOf<NonNullable<Y>>>, | ||
| ]); |
There was a problem hiding this comment.
ArfInputSet now casts inputs to Record<string, unknown> and then casts keys/options back to the generic types. This effectively disables type safety (and can hide passing an array like groupInputs or mismatched option shapes). Consider tightening the generic so inputs must be a record of input definitions (exclude groupInputs from the union), or split ArfInputSet into separate helpers for record-based inputs vs array-based sections, so this component can stay fully typed without assertions.
| const groupKey = `group-${String( | ||
| Object.values(group) | ||
| .flatMap((inputMap) => Object.keys((inputMap ?? {}) as object)) | ||
| .join("-") || index, | ||
| )}`; |
There was a problem hiding this comment.
groupInputs introduces a new recursive rendering path (nested ArfBody calls). There are existing vitest form tests in the repo, but nothing exercising grouped inputs; adding at least one test that renders a resource using groupInputs (e.g. Buildings) would help catch regressions like missing fields, incorrect layout branching (isGroup), or recursion/key issues.
| const groupKey = `group-${String( | |
| Object.values(group) | |
| .flatMap((inputMap) => Object.keys((inputMap ?? {}) as object)) | |
| .join("-") || index, | |
| )}`; | |
| const groupKey = `group-${index}`; |
| /** Used for grouping related input fields together. */ | ||
| groupInputs?: AbstractResourceFormInputs<T>[]; |
There was a problem hiding this comment.
Adding groupInputs to AbstractResourceFormInputs makes FormInputName include groupInputs (since it’s keyof AbstractResourceFormInputs<Resource>). Existing consumers (e.g. sort-filters’ getResourceFilterDefinitions) iterate over metadata.form.inputs and will now treat groupInputs (an array) as a normal input section, producing invalid filter definitions (numeric keys like "0", "1") or runtime issues. Fix by either excluding groupInputs from FormInputName (and any input-type iteration), or updating those consumers to recursively flatten groupInputs into the underlying input sections before processing.
kguzek
left a comment
kguzek
left a comment
There was a problem hiding this comment.
działa to spoko w formularzach, podoba mi się nowy syntax do grupowania inputów
| } as const; | ||
|
|
||
| /** Common address input configuration */ | ||
| /** Common address inputs configuration */ |
There was a problem hiding this comment.
tu poprawniej jest w l. pojedynczej; prędzej dopasowałbym w innych komentarzach żeby było jak tu
| const mapped = typedEntries(inputs).map(([key, options]) => | ||
| options == null ? null : mapper([key, options]), | ||
|
|
||
| const mapped = typedEntries(inputs as Record<string, unknown>).map( |
There was a problem hiding this comment.
po co taki cast jest potrzebny? nie wygląda to ładnie
| } as const; | ||
|
|
||
| /** Coordinates inputs configuration */ | ||
| const COORDINATES_INPUTS = { |
There was a problem hiding this comment.
dobrze to wyodrębnić, ale tu tez dałbym w l. pojedynczej. generalnie gdy budujemy przymiotniki z rzeczownikow (nawet mnogich tak jak coordinates) - formalnie "noun modifiers" - to wynik jest w liczbie pojedynczej
There was a problem hiding this comment.
z tego co rozumiem ten ogromny diff jest przez to że przenosimy divy wrapujące do kontrolera?
There was a problem hiding this comment.
bo troche nie chce mi się czytać całego żeby zobaczyć co się zmieniło
No description provided.