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

Handle primitive labeling in angular-material MasterListComponent #1779

Closed
JBBianchi opened this issue Jul 8, 2021 · 5 comments · Fixed by #1786
Closed

Handle primitive labeling in angular-material MasterListComponent #1779

JBBianchi opened this issue Jul 8, 2021 · 5 comments · Fixed by #1786

Comments

@JBBianchi
Copy link
Contributor

Is your feature request related to a problem? Please describe.

When using a ListWithDetail on an array of strings, the label stays undefined even if the options.labelRef is set to #.

{
  "type": "ListWithDetail",
  "scope": "#/properties/an-array-of-strings",
  "options": {
    "labelRef": "#"
  }
}

Describe the solution you'd like

I'd like the label to be the value of the item. This could be achieved by modifiying the following in master.ts

const dataIsPrimitive = d && typeof d !== 'object';
const masterItem = {
  label: get(d, labelRefInstancePath, dataIsPrimitive ? d : ''),
  data: d,
  path: `${path}.${index}`,
  schema,
  uischema: detailUISchema
};

Describe alternatives you've considered

Using "my own" version of MasterListComponent.

Framework

Angular

RendererSet

Material

Additional context

No response

@sdirix
Copy link
Member

sdirix commented Jul 8, 2021

Hi @JBBianchi, thanks for creating this issue.

We wrote the ListWithDetail renderer with arrays of objects in mind, therefore I'm not that surprised that it doesn't properly work on primitives. However it can certainly be improved to also handle that case.

Instead of calling get with a default value we should just return the primitive value (maybe stringified?) directly as the label. Also we should make sure that there is no runtime error for when options.labelRef is not given.

I'm just wondering, what is the use case of using ListWithDetail on an array of primitives? Wouldn't it be better to rely on the usual "list" renderer but adding a possibility to add and remove items?

@JBBianchi
Copy link
Contributor Author

Hello @sdirix

Instead of calling get with a default value we should just return the primitive value (maybe stringified?) directly as the label. Also we should make sure that there is no runtime error for when options.labelRef is not given.

Indeed, that makes sense.

I'm just wondering, what is the use case of using ListWithDetail on an array of primitives? Wouldn't it be better to rely on the usual "list" renderer but adding a possibility to add and remove items?

Unfortunatly, from what I understood of the renderer set table (and after trying it out), the 'simple' list isn't supported/working in angular.

By providing the following JSON Schema in the Angular seed:

JSON Schema

{
  "type":"object",
  "properties":{
    "strings":{
      "title":"Array of strings",
      "type":"array",
      "items":{ "type" : "string" }
    },
    "objects":{
      "title":"Array of objects",
      "type":"array",
      "items":{ 
        "type" : "object",
        "properties": {
          "some-prop": {
            "type": "string"
          },
          "another-prop": {
            "type": "string"
          }
        }
      }
    }
  }
}      

And the following UI Schema:

{
  "type": "VerticalLayout",
  "elements": [
    {
      "type": "Control",
      "scope": "#/properties/strings"
    }
  ]
}

It renders an empty table. (In some configuration, it will render a container with a single input but no button to add/remove items, I'll try to edit/reply if I find a way to reproduce it.) (it's the same behavior if the scope was #/properties/objects instead)

@sdirix
Copy link
Member

sdirix commented Jul 8, 2021

The renderer set overview is actually wrong there and is missing the "checkmark" for Angular Material here. Primitive values in arrays work just fine, however the renderer is missing a way to

  • create an empty array
  • add an item to the array
  • remove an item from the array

To see the existing renderer in some capacity with your provided schemas you need to also provide the data. So changing data.ts to the following does the trick.

export default {
  strings: ["string1", "string2"]
};

image

@JBBianchi
Copy link
Contributor Author

I see (the same can also be achieved by providing a default in the json schema and turning useDefaults to true in Ajv btw) but it indeed lacks the capability to add/remove items like the React Material version does. That's why I tricked and used ListWithDetail, it allows me to have those features.

@sdirix
Copy link
Member

sdirix commented Jul 9, 2021

I extracted this into a separate issue: #1782

@sdirix sdirix added this to the next milestone Jul 12, 2021
eneufeld added a commit to eneufeld/jsonforms that referenced this issue Jul 13, 2021
The MasterListComponent did only show labels for object lists.
Furthermore the options property in the uischema was mandatory.
Also the property to use as label for objects had to be provided.

The fix now handles primitives, a missing options property and
in the case of a missing `labelRef` the first primitive property
is used. If no primitive property is available the first property in
the schema is used.

Fix eclipsesource#1779
eneufeld added a commit to eneufeld/jsonforms that referenced this issue Jul 14, 2021
The MasterListComponent did only show labels for object lists.
Furthermore the options property in the uischema was mandatory.
Also the property to use as label for objects had to be provided.

The fix now handles primitives, a missing options property and
in the case of a missing `labelRef` the first primitive property
is used. If no primitive property is available the fallback text
'No label set' will be shown.

Fix eclipsesource#1779
sdirix pushed a commit that referenced this issue Jul 15, 2021
The Angular MasterListComponent did only show labels for object lists.
Furthermore the options property in the uischema was mandatory.
Also the property to use as label for objects had to be provided.

The fix now handles primitives, a missing options property and
in the case of a missing `labelRef` the first primitive property
is used. If no primitive property is available the fallback text
'No label set' will be shown.

Fix #1779
@sdirix sdirix modified the milestones: next, 3.0 Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants