Cell renderer set for vue vanilla (work in progress) #2279

davewwww · 2024-02-12T12:09:35Z

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?

netlify · 2024-02-12T12:09:40Z

❌ Deploy Preview for jsonforms-examples failed.

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

sdirix

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?

packages/vue-vanilla/src/cells/TextCell.vue

sdirix · 2024-02-14T12:24:46Z

packages/vue-vanilla/src/cells/TextCell.vue

+const input = useVanillaControl(
+  useJsonFormsControl(props),
+  (target) => target.value || undefined
+);


We should implement and use cell specific variants of the useJsonFormsControl and maybe useVanillaControl bindings, like we do in React.

By reusing the one for the renderers, we will calculate a lot of unnecessary props here (e.g. label) which are then never used.

@sdirix is the implementation in the current commit as you imagine it? I have used the useVanillaCell() and useJsonFormsCell() functions at the cells, whereby the useVanillaCell() is actually only a copy of the useVanillaControl(), so far correct?

sdirix · 2024-02-14T12:25:53Z

packages/vue-vanilla/src/controls/index.ts

+  //stringControlRendererEntry,
  multiStringControlRendererEntry,
-  numberControlRendererEntry,
-  integerControlRendererEntry,
+  //numberControlRendererEntry,
+  //integerControlRendererEntry,
  enumControlRendererEntry,
  oneOfEnumControlRendererEntry,
  dateControlRendererEntry,
  dateTimeControlRendererEntry,
  timeControlRendererEntry,
  booleanControlRendererEntry,


I think we could get rid of all the renderers in favor of cells, except maybe the boolean one.

why not the boolean one? is there any special handling? looks pretty much the same as the others.

davewwww · 2024-02-14T12:54:37Z

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.

sdirix · 2024-02-14T13:36:31Z

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 👍 Let us know if you need any support or have questions.

sdirix · 2024-02-19T15:32:50Z

@davewwww Let me know once I shall take a look again 👍

davewwww · 2024-02-19T15:42:17Z

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.

davewwww · 2024-03-06T08:34:26Z

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

sdirix · 2024-03-06T08:40:53Z

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 👍

sdirix

I had a look through the sources and commented what stuck out to me. Generally speaking this is good work and I love to see this merged in the near future ;)

packages/vue-vanilla/dev/components/App.vue

packages/vue-vanilla/src/cells/BooleanCell.vue

packages/vue-vanilla/src/cells/DateTimeCell.vue

packages/vue-vanilla/src/controls/InputControlRenderer.vue

sdirix · 2024-03-11T16:58:01Z

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.

…l at Cells. adjusted DispatchCell props.

also add a fix for eclipsesource#2156 at dateTimeCell: use v-model instead of value and add missing seconds at timeCell

…ead of props.data

sdirix · 2024-04-15T16:00:39Z

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

sdirix · 2024-04-22T20:03:29Z

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.

sdirix self-requested a review February 14, 2024 10:24

sdirix reviewed Feb 14, 2024

View reviewed changes

davewwww marked this pull request as draft February 15, 2024 09:25

davewwww force-pushed the feature/cell-render-set branch from 0677ac8 to 3c21c93 Compare February 15, 2024 13:46

davewwww requested a review from sdirix February 19, 2024 15:42

davewwww mentioned this pull request Feb 22, 2024

Radio Group Renderer for Vue Vanilla (WIP) #2292

Draft

sdirix reviewed Mar 11, 2024

View reviewed changes

davewwww added 5 commits March 18, 2024 10:24

rebase onto master

b4baa52

create useVanillaCell function and use it instead of useVanillaContro…

6a52e77

…l at Cells. adjusted DispatchCell props.

replace all control renderers with a cell equivalent.

8c09746

also add a fix for eclipsesource#2156 at dateTimeCell: use v-model instead of value and add missing seconds at timeCell

use rendererProps() for defineProps(), use input.cell.value.data inst…

029308e

…ead of props.data

hand over only some keys instead of binding control

893774b

davewwww force-pushed the feature/cell-render-set branch from 9850f2f to 893774b Compare March 18, 2024 09:25

davewwww requested a review from sdirix March 18, 2024 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cell renderer set for vue vanilla (work in progress) #2279

Cell renderer set for vue vanilla (work in progress) #2279

davewwww commented Feb 12, 2024

netlify bot commented Feb 12, 2024 •

edited

Loading

sdirix left a comment •

edited

Loading

sdirix Feb 14, 2024

davewwww Feb 15, 2024

sdirix Feb 14, 2024

davewwww Feb 15, 2024 •

edited

Loading

davewwww commented Feb 14, 2024 •

edited

Loading

sdirix commented Feb 14, 2024 •

edited

Loading

sdirix commented Feb 19, 2024

davewwww commented Feb 19, 2024 •

edited

Loading

davewwww commented Mar 6, 2024

sdirix commented Mar 6, 2024 •

edited

Loading

sdirix left a comment

sdirix commented Mar 11, 2024 •

edited

Loading

sdirix commented Apr 15, 2024

sdirix commented Apr 22, 2024

Cell renderer set for vue vanilla (work in progress) #2279

Are you sure you want to change the base?

Cell renderer set for vue vanilla (work in progress) #2279

Conversation

davewwww commented Feb 12, 2024

netlify bot commented Feb 12, 2024 • edited Loading

❌ Deploy Preview for jsonforms-examples failed.

sdirix left a comment • edited Loading

Choose a reason for hiding this comment

sdirix Feb 14, 2024

Choose a reason for hiding this comment

davewwww Feb 15, 2024

Choose a reason for hiding this comment

sdirix Feb 14, 2024

Choose a reason for hiding this comment

davewwww Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

davewwww commented Feb 14, 2024 • edited Loading

sdirix commented Feb 14, 2024 • edited Loading

sdirix commented Feb 19, 2024

davewwww commented Feb 19, 2024 • edited Loading

davewwww commented Mar 6, 2024

sdirix commented Mar 6, 2024 • edited Loading

sdirix left a comment

Choose a reason for hiding this comment

sdirix commented Mar 11, 2024 • edited Loading

sdirix commented Apr 15, 2024

sdirix commented Apr 22, 2024

netlify bot commented Feb 12, 2024 •

edited

Loading

sdirix left a comment •

edited

Loading

davewwww Feb 15, 2024 •

edited

Loading

davewwww commented Feb 14, 2024 •

edited

Loading

sdirix commented Feb 14, 2024 •

edited

Loading

davewwww commented Feb 19, 2024 •

edited

Loading

sdirix commented Mar 6, 2024 •

edited

Loading

sdirix commented Mar 11, 2024 •

edited

Loading