eclipsesource / jsonforms-vuetify-renderers

https://jsonforms-vuetify-renderers.netlify.app/
Other
27 stars 26 forks source link

Add more renderers #40

Closed kchobantonov closed 2 years ago

kchobantonov commented 2 years ago

add FileControlRenderer, TemplateLayoutRenderer, TemplateLabelRenderer, examples, fix toggle control

netlify[bot] commented 2 years ago

Deploy Preview for jsonforms-vuetify-renderers ready!

Name Link
Latest commit 097ba294fa4ad3b1c239d72441f53a838a1af1bd
Latest deploy log https://app.netlify.com/sites/jsonforms-vuetify-renderers/deploys/627d061d0845dc000874e1eb
Deploy Preview https://deploy-preview-40--jsonforms-vuetify-renderers.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 settings.

kchobantonov commented 2 years ago

@sdirix can you please review

kchobantonov commented 2 years ago

I did not do any runtime compiling in Vue yet so I'm no expert here but in general the code looks fine 👍 .

When a user wants a customized layout, we usually recommend to implement a custom renderer for their use cases. Using these template renderers they can solve some of their use cases via the UI Schema.

The downsides are:

  • This feature is not documented and it is hard to find and use. I would be surprised if anyone else than yourself will actually use this feature
  • Every user of the renderer set gets a dependency to vue-template-compiler into their application even if they don't need it.

Therefore I was wondering whether we should offer the renderers separately to the main renderer set. We did this in the past for React Material UI too. Of course it makes it even harder to use then. Also we might then want to remove the peer dependency listing to vue-template-compiler.

The FileControlRenderer I'm even more unsure about. This is a very specific implementation, completely decoupled from JSON Schema. Users must be "lucky" that they have exactly the use case it was implemented for. I feel like this renderer would be best documented as an example renderer within the community forum or on the website.

What do you think?

The reason why the template component is going to be used more is the fact that for slight UI changes that are needed like adding some visual UI elements to enhance the look of the UI form then we do not need to do another component and yet another one and etc. Having one that will deal with that in a generic way should be more preferrable. I agree with you that we will add the compiler into the final build that I do not think that this will make the final JS file much more larger and if I have to do that with new components the JS will be larger anyway. Perhaps we might think to support 2 versions of that - one light without those components and one with the full support ?

Also this kind of support is especially useful when we deal with jsonform integration like this project of mine https://github.com/kchobantonov/camunda-jsonforms-plugin where when the JS is built and integrated with camunda for example then it is going to be easy to add forms with custom UI without recompilation of the whole UI library (jsonforms + vuetify renderers + custom renderers)

I agree that FileControlRender could be having more documentation how to use it even though we have section especially for it in the examples. Those renderer are in nature generic and not specific to jsonforms + camunda integration but still this is needed for that integration. If you prefer this to not be included into that then we can just remove it I guess but I thought that those are generic enough in nature to be in the library itself.

kchobantonov commented 2 years ago

@vue/composition-api is used now on only one file, the example for using the FileControlRenderer is in the examples under /#/example/control, @sdirix please review the changes and check my comment regarding the template renderer - I believe it would be a good addition to the set but again if you believe that is not the case then lets try to have maybe 2 versions - full and light or something like that.

kchobantonov commented 2 years ago

@sdirix also what do you think about a renderer that does not do any rendering but actually fetches rest api data - for example when we want to show a drop down UI with items retrieved from rest api instead from the json schema or ui schema - take a look at https://github.com/kchobantonov/camunda-jsonforms-plugin/blob/master/plugin-ui/packages/common/src/renderers/DataProviderRenderer.vue and https://github.com/kchobantonov/camunda-jsonforms-plugin/blob/master/plugin-ui/packages/common/src/renderers/DataProviderSelectRenderer.vue. It would be better of course if all our renderers support this remote data as the same way how we do that to get the data from the json schema or ui schema instead for me to have specialized drop-down renderer just to be able to use that remote data. What do you think ? And in order this to work I have a custom dispatcher https://github.com/kchobantonov/camunda-jsonforms-plugin/blob/master/plugin-ui/packages/common/src/renderers/DataDispatchRenderer.vue

kchobantonov commented 2 years ago

@sdirix can you review again.

sdirix commented 2 years ago

The reason why the template component is going to be used more is the fact that for slight UI changes that are needed like adding some visual UI elements to enhance the look of the UI form then we do not need to do another component and yet another one and etc. Having one that will deal with that in a generic way should be more preferrable. I agree with you that we will add the compiler into the final build that I do not think that this will make the final JS file much more larger and if I have to do that with new components the JS will be larger anyway. Perhaps we might think to support 2 versions of that - one light without those components and one with the full support ?

I'm just looking for a way to not force the template compiler dependency on users who don't need it. From the typical use cases I experience these would be most users ;) Maybe it's sufficient to just not include the template renderers in the vuetifyRenderers export but export them separately. Maybe webpack is then able to tree-shake them away if not used. Should be checked.

Also this kind of support is especially useful when we deal with jsonform integration like this project of mine https://github.com/kchobantonov/camunda-jsonforms-plugin where when the JS is built and integrated with camunda for example then it is going to be easy to add forms with custom UI without recompilation of the whole UI library (jsonforms + vuetify renderers + custom renderers)

Yes makes sense for this use case :+1:

I agree that FileControlRender could be having more documentation how to use it even though we have section especially for it in the examples. Those renderer are in nature generic and not specific to jsonforms + camunda integration but still this is needed for that integration. If you prefer this to not be included into that then we can just remove it I guess but I thought that those are generic enough in nature to be in the library itself

It's a bit special in the sense that:

I think it's a good example for a custom renderer but I don't really see it as part of the "off-the-shelf" renderers as I don't see many users requiring exactly that implementation of file upload.

also what do you think about a renderer that does not do any rendering but actually fetches rest api data - for example when we want to show a drop down UI with items retrieved from rest api instead from the json schema or ui schema

Yes a drop down / autocomplete selection communicating with some end point is a very common use case and probably the custom renderer which we wrote most often for customer projects. Having said that, in practice there is hardly any shareable code there. One almost always need some kind of code there anyway to navigate the respective REST API and transforming it to something which can be shown in the UI. Usually these projects code their one REST custom control which knows how the respective end point is determined and how to communicate with that and then they reuse it all over their forms.

Again there is the question of whether it makes sense to add such a renderer in some form to the "off-the-shelf" renderer set. There is not really standard for that in JSON Schema (There is the JSON Schema hyper schema but it seems to no longer being updated. I don't actually know whether it would cover this use case or not)

If one decides for yes, then we need to think about how this could actually be used and whether any user will actually save time by using the provided mechanism instead of letting them just handle it themselves. From my experience I would lean to not providing such a renderer as all the renderers of such sort I know so far are different enough in behavior that they would require code adaptions anyway, e.g. think about initialization, error cases, reponse times, caching, virtualization, what to do on selection, included graphics, internationalization, query parameters etc.

Of course if you need something like this for Camunda then this would make a great custom renderer there, e.g. users then specify the URL of their enum options somewhere and then just need to make sure the return values match. You then need to decide what to actually store in the data.

This is still just my opinion. I'll discuss this with the team and come back to you with their conclusion.


As this PR also includes fixes for existing renderers you might want to open a separate PR just with them. We can then review/merge them much more quickly.


Thanks for the contribution, contribution ideas and this interesting discussion ;)

kchobantonov commented 2 years ago

Just one note that the json schema does define binary formats so the FileRenderer will be perfect match for it - Media: string-encoding non-JSON data.

Regarding the data renderer which fetch data as well as the template renders it definetely be used by someone that only depends on the library without forcing him to extend it using custom renderers. I do not think that every project that uses jsonforms always have a development team that can deal with extending the jsonforms library but I see your point as well.

Maybe in future it will help adding issues first to the project and only when approved (e.g. there is an intent to merge such features) then to add the implementation for it otherwise it is waste of time IMO.

sdirix commented 2 years ago

Just one note that the json schema does define binary formats so the FileRenderer will be perfect match for it - Media: string-encoding non-JSON data.

Somehow I missed that. If the file renderer works against this spec then I'm all for it :+1:

kchobantonov commented 2 years ago

Just one note that the json schema does define binary formats so the FileRenderer will be perfect match for it - Media: string-encoding non-JSON data.

Somehow I missed that. If the file renderer works against this spec then I'm all for it 👍

Yes it is against the spec - the only thing that is not covered by the spec is that the file name can also be included into the data URL where usually data URL does not have such behavior but for obvious reasons there are cases where this is needed.

kchobantonov commented 2 years ago

Also one final argument for the Vue template compiler inclusion and the template renderers (including the new label template renderer) is that at the moment all text that is displayed on jsonforms is static (using the LabelRenderer). What if for example I would want to show a label that included a data from the json that was obtained from the remote system and instead of modifying that on the form I want to display it ? Like - 'Welcome {{ firstName }} {{ lastName }}' or perhaps 'Your order was accepted and the total is {{ amount | toCurrency }} ' where the toCurrency was registered filter in Vue.

yaffol commented 2 years ago

Thinking about use cases for a more flexible way of defining things like labels, I was considering the example of the 'add new' button on array layouts. In my case, I want the button to be at the bottom of the control, on the left hand side, and to say 'Add new' instead of just '+'.

Now this can be made configurable, and I am making a PR to do this, but that's a bit heavy-weight for a small change. Again I could make a custom renderer, so perhaps this example is a little contrived, but I think it's still a useful example.

So I can see a situation where I could make use of the ability to customise small parts of renderers, like a button, without overriding the whole thing.

I would also want to be using the i18n functions to handle custom strings, which is an important consideration. I need to look into whether that is possible. And indeed, there is a mechanism to potentially customise strings there - I want to move to a more complete i18n system such as FormatJS or Fluent-Vue, where messages could be constructed from a known message format and parameters passed for constructing the translated string. That would also permit the use case of a custom string, though it does push the abstraction outside the library, into your i18n files.

The ability to make these sorts of customisations does feel 'vue-like' and reminds me of using slots.

Avoiding a hard (or possibly apparently hard, but tree-shaken) dependency on the template renderer would be good, as it's a bit of a red flag when just scanning a package.json - but I'm not sure there's a way around it. Some clarity in the docs about what the impact actually is, especially if tree-shaking does remove it if you don't use the feature, would go a long way to mitigating this, in my opinion.

sdirix commented 2 years ago

I'm fine with merging if we avoid the dependency to the template compiler by default (for example if it's tree shaked when not needed). So this must be tested.

kchobantonov commented 2 years ago

closing this PR since we have other PRs that included this one