Skip to content
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

feat: improve support for minItems and maxItems for array layout and control #2387

Merged
merged 6 commits into from
Dec 6, 2024

Conversation

Typiqally
Copy link
Contributor

@Typiqally Typiqally commented Nov 11, 2024

Added improved support for the minItems and maxItems properties, to allow for more fine-grained control and validation in ArrayControl and ArrayLayout applications. I have also added additional tests to cover these properties.


While working on this, I realized that the current handling of array (or items) schemas is too restrictive for custom implementations. Specifically, the schema is being completely replaced by the items schema, which typically only includes a basic type description and hides important details like the min and max item properties from the original array schema.

export const mapStateToArrayControlProps = (
state: JsonFormsState,
ownProps: OwnPropsOfControl
): StatePropsOfArrayControl => {
const { path, schema, uischema, label, ...props } =
mapStateToControlWithDetailProps(state, ownProps);
const resolvedSchema = Resolve.schema(schema, 'items', props.rootSchema);
const childErrors = getSubErrorsAt(path, resolvedSchema)(state);
return {
...props,
label,
path,
uischema,
schema: resolvedSchema,
childErrors,
renderers: ownProps.renderers || getRenderers(state),
cells: ownProps.cells || getCells(state),
};
};

To address these limitations, I see two possible approaches:

  1. Add a parentSchema property to preserve the original schema, keeping the existing behavior while easing the constraints. This might introduce some confusion around naming.
  2. Store the original schema in the schema property and put the resolved schema in a new arraySchema or itemsSchema property. However, this would change the current behavior, requiring migration.

I’d appreciate your thoughts on these options.

@CLAassistant
Copy link

CLAassistant commented Nov 11, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

netlify bot commented Nov 11, 2024

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit f2fb96c
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/6751e39983b4ca0008c54267
😎 Deploy Preview https://deploy-preview-2387--jsonforms-examples.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Typiqally Typiqally closed this Nov 11, 2024
@Typiqally Typiqally reopened this Nov 11, 2024
@sdirix
Copy link
Member

sdirix commented Nov 11, 2024

I agree that the current situation is not optimal. I would like to suggest to add a new prop arraySchema in which the schema referring to the array is handed over. For now we should not adapt the schema prop, as this will certainly lead to backward compatibility issues with custom renderers.

We could think of a migration path with a major release or just keep the current prop naming, even if it's not ideal.

@Typiqally can you add this arraySchema prop?

@Typiqally
Copy link
Contributor Author

Typiqally commented Nov 11, 2024

I have added the arraySchema prop.

The minItems and maxItems props have become redundant because they can now be accessed via the arraySchema prop. I propose reverting the previous changes related to these props and deprecating the existing minItems prop in ArrayLayout to warn future custom implementations about depending on this prop.

What do you think @sdirix?

@sdirix
Copy link
Member

sdirix commented Nov 11, 2024

Yes, let's refactor our code so we use arraySchema where appropriate. If a binding did hand over minItems then we should still do that to not break anyone on the update.

@Typiqally
Copy link
Contributor Author

@sdirix, I have removed the props and deprecated the original minItems property in favour of using arraySchema.minItems. I don't see an usage of this property anywhere in the internal packages. Only the Vuetify package seems to be using something related to minItems, but they get around these props by resolving the schema manually in the component itself.

@sdirix sdirix self-requested a review December 4, 2024 10:38
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ❤️

@sdirix sdirix merged commit 43a9ea7 into eclipsesource:master Dec 6, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants