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

Add context for array operation #2317

Merged
merged 1 commit into from
Apr 11, 2024
Merged

Conversation

eneufeld
Copy link
Member

@eneufeld eneufeld commented Apr 9, 2024

Currently it is very hard to figure out in the middleware what array operation happend to lead to the current array state. There are cases were this information is valuable. To support this all update actions can now have a 'context'. Here we added a specific 'UpdateArrayContext'.

Contributed on behalf of STMicroelectronics

@eneufeld eneufeld requested a review from sdirix April 9, 2024 10:08
Copy link

netlify bot commented Apr 9, 2024

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit 2fcf638
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/661564a6ecd1d3000834bba3
😎 Deploy Preview https://deploy-preview-2317--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.

@coveralls
Copy link

coveralls commented Apr 9, 2024

Coverage Status

coverage: 84.795% (-0.009%) from 84.804%
when pulling 2fcf638 on eneufeld:arrayContext
into c74b8ba on eclipsesource:master.

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.

Looks good and thanks for adding the tests. Please have a look at my comments.

packages/core/src/actions/actions.ts Show resolved Hide resolved
packages/core/src/actions/actions.ts Outdated Show resolved Hide resolved
packages/core/src/actions/actions.ts Outdated Show resolved Hide resolved
Comment on lines 80 to 81
const firstValueType = typeof context.values[0];
return context.values.every((v) => typeof v === firstValueType);
Copy link
Member

Choose a reason for hiding this comment

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

I think this check is not necessary. JSON Schema supports different types in arrays.

Copy link
Member Author

Choose a reason for hiding this comment

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

but we don't support this in jsonforms, so all newly added values must be of the same type
I'm fine removing this check

Copy link
Member

Choose a reason for hiding this comment

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

We don't support it in JSON Forms in our current renderers but there is no restriction for that in JSON Schema and custom/future renderers might support that.

Currently it is very hard to figure out in the middleware what array
operation happend to lead to the current array state.
There are cases were this information is valuable.
To support this all update actions can now have a 'context'.
Here we added a specific 'UpdateArrayContext'.

Contributed on behalf of STMicroelectronics
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.

Works for me!

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

Successfully merging this pull request may close these issues.

4 participants