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

fix complex renderers #2392

Merged
merged 8 commits into from
Dec 9, 2024

Conversation

kchobantonov
Copy link
Contributor

update demo app, fix oneOf, allow passing slots to the dispatcher renderer, use vuetify widgets for integer and number inputs

…derer, use vuetify widgets for integer and number inputs
Copy link

netlify bot commented Nov 26, 2024

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit 5974eca
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/675373e52c3bd800074815a4
😎 Deploy Preview https://deploy-preview-2392--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.

@kchobantonov
Copy link
Contributor Author

@sdirix please review - this fixes oneOf renderer where it could clear out the whole data - check for example - oneOf - Preselection for Primitives (Issue 1273 ) example and try to switch between diferent oneOf when they are populated -once the dialog is clicked to apply the selectio the data will not be an object

the other changes are some stuff that I'm looking to do and are part of the excercise - be able to have a generic editor for jsonschema - e.g. the schema to be the jsonschema meta model and then to be able to create a new jsonschema with that - other are other stuff that I'm working on fixing but are not yet ready - like having a MixedControl which can handle mixed types by asking you to select the type of the element before showing you the actual control to work with the data when creating a new data and if the data is present then automatically selecting the correct type.

One thing that I'm having an issue is with the jsonforms core resolveSchema method which when it can't find the property it will return undefined and in the case of dynamic schema you do not really have a structure

{ "type: "object", "additionalProperties": true }

and we try to find a property in that schema then the resolveSchema does not give me the schema that I have provided. Locally I'm doing some hacks which I do not really like for example attaching a Symbol keys to the schema that points to a function that can be then invoke by the resolveSchema in that particular case but that looks too hacky. If you have any suggestions please let me know.

Also are you interested in such renderer - MixedRenderer or maybe we can call it UnionRenderer - anyway let me know if such one could go to the jsonform vuetify render repo - otherwise I won't spent much time to even prepare such PR

@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.

The changes look mostly good! Can you take a look at the CI checks and fix them?

const input = useVuetifyControl(useJsonFormsControl(props), adaptValue);
// preserve the value as it was typed by the user - for example when the user type very long number if we rely on the control.data to return back the actual data then the string could appear with exponent form and etc.
// otherwise while typing the string in the input can suddenly change
const inputValue = ref((input.control.value.data as string) || '');
Copy link
Member

Choose a reason for hiding this comment

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

Good idea with the simplications

@@ -141,7 +142,7 @@ export const useVuetifyControl = <
},
I extends {
control: ComputedRef<T>;
} & DispatchPropsOfControl,
} & (DispatchPropsOfControl | DispatchPropsOfMultiEnumControl),
Copy link
Member

Choose a reason for hiding this comment

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

Seems weird to add this type here. What is the rationale?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so that we can use that useVuetifyControl with the EnumArrayRenderer instead of creating very similar method just for that renderer

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I see. The core issue is that we do not expose handleChange to these controls although I would have expected it. See for example the React renderers in which we not only do the specialized addIterm etc. but also expose the usual handleChange.

I'm fine with this for this PR but we should rework in the future I think

@kchobantonov kchobantonov requested a review from sdirix December 6, 2024 22:08
@kchobantonov
Copy link
Contributor Author

@sdirix all fixed and those that needed clarification please find those in the corresponding sections.

@coveralls
Copy link

Coverage Status

coverage: 82.504%. remained the same
when pulling 5974eca on kchobantonov:fix-complex-renderers
into d705e9d 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.

Works for me

@sdirix sdirix merged commit ce42184 into eclipsesource:master Dec 9, 2024
8 checks passed
@kchobantonov kchobantonov deleted the fix-complex-renderers branch December 9, 2024 15:26
@sdirix sdirix added this to the 3.5 milestone Jan 10, 2025
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.

None yet

3 participants