jsonforms

jsonforms copied to clipboard

regarding: #1720

Hi,

I have tried to implement a proposal for the missing cell feature in vue vanilla.

What do you think, does it work so far?

❌ Deploy Preview for jsonforms-examples failed.

Name	Link
🔨 Latest commit	893774b625d60d3411ff8841a0e5cf5553d4e47e
🔍 Latest deploy log	https://app.netlify.com/sites/jsonforms-examples/deploys/65f8089a4083d0000718c7ea

Hi @davewwww,

I discussed with the team. We very much like leveraging the cells mechanism and using it similar to the React Vanilla approach. If this PR is properly finished, then we don't even need the ControlWrapper customization mechanism anymore. Instead one can simply override the InputControlRenderer with the same end result. As this is the cleaner approach, we would prefer that.

Would you be willing to finish this contribution, i.e. removing most of the current control renderers which we have, replacing them with a cell variant?

That is of course an interesting thought. So far I use the ControlWrapper in each of my custom renderers. But perhaps it would still be a good idea to keep the ControlWrapper in place if a customer don't want to go with this cells but want to stick with your "old" custom renderer. Topic backwards compatibility

And yes, if you give the GO, then I would continue to work on it.

Hi @davewwww, I discussed with the team. We very much like leveraging the cells mechanism and using it similar to the React Vanilla approach. If this PR is properly finished, then we don't even need the ControlWrapper customization mechanism anymore. Instead one can simply override the InputControlRenderer with the same end result. As this is the cleaner approach, we would prefer that. Would you be willing to finish this contribution, i.e. removing most of the current control renderers which we have, replacing them with a cell variant?

That is of course an interesting thought. So far I use the ControlWrapper in each of my custom renderers. But perhaps it would still be a good idea to keep the ControlWrapper in place if a customer don't want to go with this cells but want to stick with your "old" custom renderer. Topic backwards compatibility

• Refactoring the existing renderers should be backward compatible without any effect for existing users (besides having to hand over the cells too) as we don't expect anyone importing them manually.
• We can keep the ControlWrapper for the InputControlRenderer so that anyone who imported the ControlWrapper can still use it
• If users have existing custom renderers, they will continue to work without issues, even if not using the cell approach.

I would just avoid introducing the new control-wrapper injection mechanism as a new additional way of customizing as it's no longer needed. If you have existing custom renderers using the control-wrapper and you would like to use your own customized one, then you can simply use your own implementation of control-wrapper for them. Then you register an additional custom InputControlWrapper using your customized control-wrapper for all the off-the-shelf use cases you didn't customize before.

And yes, if you give the GO, then I would continue to work on it.

Please go ahead, we're looking forward to this feature :+1: Let us know if you need any support or have questions.

@davewwww Let me know once I shall take a look again :+1:

there is currently a error at the building that i wanted to investigate. but i don't really understand it, maybe you have an idea.

Error: [@vue/compiler-sfc] No fs option provided to `compileScript` in non-Node environment. File system access is required for resolving imported types.

Besides that, all cells are converted and you can have a look at it. especially the time & datetime cells.

@sdirix What do you think about my proposal? I would like to contribute more cells & renderers for vue vanilla .

Hi @davewwww, we had less resources than usual in the last weeks which is why I did not have time yet to look at the PR again. I will do so as soon as possible, probably within a week.

If possible it would be great if you could make sure that the build is green. Then if the code is fine, we can immediately merge. Edit: Just saw that you mentioned the error already above. I will take a look when I review the PR :+1:

there is currently a error at the building that i wanted to investigate. but i don't really understand it, maybe you have an idea.

Error: [@vue/compiler-sfc] No fs option provided to `compileScript` in non-Node environment. File system access is required for resolving imported types.

Besides that, all cells are converted and you can have a look at it. especially the time & datetime cells.

This error likely comes from the type-based defineProps generation you are trying to use (i.e. const props = defineProps<CellProps>();). It seems it's not so stable of a feature yet as I found multiple unresolved error reports of people running into problems with that. My suggestion would be to go back to the regular prop definitions, as we use them the other Vue components, like here.

Update: I will re-review this PR again this week

I like the cells but we need to fix the build errors. I started refactoring the cells but did run out of time. I think we could try not using the setup script but declaring them like we do the regular components. Then I would remove the tester from each .vue file and declare them in a separate file. The combined tester in the .vue files lead to all kind of errors in the past anyway with Vite.