-
Notifications
You must be signed in to change notification settings - Fork 32
Bind List/MultipleSelect/Checkboxes to a repeating group #3236
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Johanne Lie <joli@patentstyret.no> Co-authored-by: adamhaeger <adamgullerud@gmail.com>
# Conflicts: # src/layout/List/ListComponent.tsx
…datory for bound values
# Conflicts: # src/layout/List/ListComponent.tsx
# Conflicts: # src/layout/MultipleSelect/MultipleSelectComponent.tsx # src/layout/MultipleSelect/config.ts
|
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.
I think we should go the extra mile to support varying depths of properties inside these rows, so that we can support any data model instead of apps having to tailor their data model to our functionality. We don't currently have any such limitations elsewhere in the product (at least that I know), and these limitations in List
was previously removed in #3136.
We can also team up to pair-program the rest of this, if you want, or if my suggestions were unclear. 🙌
//TODO: excluding from List now?? | ||
} else if (binding !== 'group' && binding !== 'checked' && binding !== 'label' && binding !== 'metadata') { | ||
next[binding] = row[binding]; | ||
} |
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 the TODO here still relevant?
Also, i find this checking for other bindings brittle. When we introduce a new binding to Checkboxes
or MultipleSelect
, this will have to be updated (and then possibly be a breaking change for existing List
component configurations). It will be difficult to notice (TypeScript won't warn us in any way, and no tests will catch this unless configured along with a group
binding). I would prefer something like:
if (componentType !== 'List' && binding === 'simpleBinding') {
...
} else if (componentType === 'List' && binding !== 'group' && binding !== 'checked') {
...
}
Also, same goes here, use dot.str()
to put values in the object when supporting any-depth properties:
dot.str(relativeBinding, newValue, next);
const simpleBindingPath = dataModelBindings.simpleBinding?.field.split('.'); | ||
const groupBinding = ctx.lookupBinding(dataModelBindings?.group); | ||
const items = groupBinding[0]?.items; | ||
const properties = | ||
items && !Array.isArray(items) && typeof items === 'object' && 'properties' in items | ||
? items.properties | ||
: undefined; | ||
|
||
if (!(properties && simpleBindingPath[1] in properties)) { | ||
errors.push(`The property ${simpleBindingPath[1]} must be present in group`); | ||
} |
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.
This also only supports one level deep structures. I recently fixed the validatior in src/layout/List/index.tsx
to work with any levels of properties, you could take a look at how that does it.
import { NodesInternal } from 'src/utils/layout/NodesContext'; | ||
import type { NodeValidationProps } from 'src/layout/layout'; | ||
|
||
export function ListLayoutValidator(props: NodeValidationProps<'List'>) { |
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.
Isn't this duplicated now? Might be better to make one that works the same for all components. If I'm not mistaken, it's as simple as:
export function ListLayoutValidator(props: NodeValidationProps<'List'>) { | |
export function LayoutValidatorForGroupAndChecked(props: NodeValidationProps<'List' | 'Checkboxes' | 'MultipleSelect'>) { |
validateDataModelBindings(ctx: LayoutValidationCtx<'MultipleSelect'>): string[] { | ||
return this.validateDataModelBindingsSimple(ctx); | ||
const errors: string[] = []; |
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.
Instead of copy-pasting this, you can also extract it to a function in a separate file and import it. 🙏
…t/Multiselect and Checkboxes
…t/Multiselect and Checkboxes
Description
Bind List/MultipleSelect/Checkboxes to a list of Objects/a repeating group.
Added a group, checked and deletionStrategy property to the List/MultipleSelect/Checkboxes, to support saving to a list of Objects and to support toggling soft deletion.
Related Issue(s)
Verification/QA
Manual functionality testing
Automated tests
UU/WCAG (follow these guidelines until we have our own)
User documentation @ altinn-studio-docs
Support in Altinn Studio
Sprint board
Labels
kind/*
andbackport*
label to this PR for proper release notes grouping