describo / crate-builder-component

A VueJS UI component to build an RO-Crate
MIT License
6 stars 3 forks source link

Proposal - revised layouts configuration in profile #72

Closed marcolarosa closed 10 months ago

marcolarosa commented 10 months ago

@beepsoft

The current layout configuration in the profiles as designed by the previous lead, doesn't make sense when an entity has multiple types. Consider the root dataset which must have type = 'Dataset' but can also have other types like 'RepositoryCollection' or 'RepositoryObject'.

In this case, the profile author needs to define layouts as:

...
layout: {
   "Dataset": {},
   "Dataset, RepositoryCollection:" {},
   "Dataset, RepositoryObject:" {},
}

This is not only redundant but also prone to error as an order is imposed that may not be how the entity type is written, e.g. it could be [ RepositoryObject, Dataset ] (which is valid) and then doesn't match a layout.

I think a better option is to keep the layout section to define the metadata of the groups but tag each property as being part of one or more groups.

layouts: {
   about: { label: 'Core Metadata', description: '....some thing about it', },
   groupXYZ: { label: 'Things about stuff', .... }
}

Then in the class definitions:

classes: {
   Dataset: {
       inputs: [
         {
           id: '...',
          group: 'about', 
         },
         {
           id: '...',
          group: 'groupXYZ',
         },
    },
    RespositoryCollection {
       inputs: [
         {
           id: '...',
          group: 'about', 
         },
         {
           id: '...',
          group: 'groupXYZ', 
         },
    },

A property could also be hidden with a hide tag and this also ties in to #22 with properties declaring themselves to be readonly.

Then in the class definitions:

classes: {
   Dataset: {
       inputs: [
         {
           id: '...',
          hide true, 
         },
         {
           id: '...',
          readonly: true,
         },
    },

This is obviously a big change to how profiles currently work. What do you think?

marcolarosa commented 10 months ago

@beepsoft

Can you please check out the branch https://github.com/describo/crate-builder-component/tree/rethink-tabbing and test it out for me?

This branch has the revised layout configuration I talk about above, handling both hidden and readonly (#22) properties. It also has a fix for #67 which was happening because the components were being refreshed on crate change; hence the behaviour you noticed.

If you think it's ok I'll merge it in and it will resolve this ticket, #22 and #67.

beepsoft commented 10 months ago

It definitely make sense to make the model "normalized" like this.

Now you refer to the layout tabs as group in the input definitions. Wouldn't it make sense to use the term groups instead of layouts then? I think "group/groups/grouping" is a better term for what we try to achieve here, than "layout". So, we can say that an input belongs to an input group and we have (input) group tabs to display inputs in a group.

I can see you also have a top level hide property defined (from the nyingarn-item-profile.json).

    "hide": {
        "Dataset": ["identifier", "hasPart", "licence"]
    },

How does this relate to the hide property of an input?

Finally, I really like that the "about" and "overflow" groups are customizable as well.

marcolarosa commented 10 months ago

It definitely make sense to make the model "normalized" like this.

Now you refer to the layout tabs as group in the input definitions. Wouldn't it make sense to use the term groups instead of layouts then? I think "group/groups/grouping" is a better term for what we try to achieve here, than "layout". So, we can say that an input belongs to an input group and we have (input) group tabs to display inputs in a group.

I take your point and I did think about this but I kept layout because that's what it defines: a UI layout. So from that perspective I'd like to keep it that way. And I went with the singular rather than the plural (the current config is plural) as it is a singular definition for the profile.

I can see you also have a top level hide property defined (from the nyingarn-item-profile.json).

    "hide": {
        "Dataset": ["identifier", "hasPart", "licence"]
    },

That's a mistake! I need to remove it. Thanks for pointing it out.

How does this relate to the hide property of an input?

Finally, I really like that the "about" and "overflow" groups are customizable as well.

Great. They were always customisable but the code to do it is much simpler now which is nice.

I'm keen to merge this tomorrow if I have time.

marcolarosa commented 10 months ago

This needs some more thought and also has a spillover effect on #67.

In the case where a layout is applied, when the user navigates to another entity in the graph that entity might not define any properties visible on that tab. So, in that case, when the tab doesn't change, the user see's a blank screen.

So there are two issues here:

  1. Should the tab be set to About when the entity changes? I now remember this is why I changed it to work that way.

    • Perhaps this needs to be handled by a component property where in your use case you could specify no tab change and in mine I would say navigate to about on entity change.
  2. How do we define layout exclusions or inclusions in the profile? e.g. apply the layout when on certain entities regardless of what @types are in the array and not on others.

https://github.com/describo/crate-builder-component/assets/2639995/daccd536-9080-4da7-a54e-fccb5bf96ee5

In the screencast the layout applied makes sense for dataset but not for file. And by not changing tab when navigating to entity the user loses the context of the navigation.

The reason for the original design is that the layout was applied for given entity types. But then when the type array changed, if there was no configuration for it it would disappear. In the example of a dataset that then had repositoryCollection added this is not the intended behaviour.

marcolarosa commented 10 months ago

@beepsoft I think I have a solution.

Basically, the layout property in the profile is back to layouts (plural) and it's an array of definitions as described above but with one extra property, appliesTo. Here's the snippet from the nyingarn profile:

    "layouts": [
        {
            "appliesTo": ["Dataset"],
            "about": { "label": "About", "description": "" },
            "source": { "label": "Original Source Information", "description": "" },
            "who": { "label": "Who", "description": "" },
            "permissions": {
                "label": "Permissions",
                "description": ""
            },
            "overflow": {
                "label": "Other"
            }
        },
        {
            "appliesTo": ["Language"],
            "about": { "label": "About", "description": "" },
            "source": { "label": "More information", "description": "" },
            "overflow": {
                "label": "Other"
            }
        }
    ],

In it I have two layouts; the first for entities of type Dataset and the second for entities of type Language. When the component reads this it will apply the first to any entity with type Dataset in the array (so it matches on type = ['Dataset'] and type = ['Dataset', 'OtherClass', 'Whatever']. Being an array, appliesTo can have any number of types in it and so long as one matches, it will be applied.

Ordering in the profile is important. If an entity is of type [Dataset, Language] (the entity type order doesn't matter), the first matching layout in the profile will be used. I think this is reasonable behaviour.

To get around the tab change behaviour I've also added another component option: resetTabOnEntityChange. Default is true meaning that the tab will go back to about on entity change. Try it with it the complex item and nyingarn profile and see that when you click a language the tabs will change and the active tab will be about.

beepsoft commented 10 months ago

Regarding the profile update, it looks good to me, with single types entities this seems semantically the same as before.

I tried setting resetTabOnEntityChange (:reset-tab-on-entity-change="true", :reset-tab-on-entity-change="false"), but cannot really see what happens or should happen. I see that changing the crate content keeps the current tab position, but you are describing something else, I guess. Your use case is something about 2 entities having the same named layout tab, and in that case you stick to that tab when changing between the two entities?

marcolarosa commented 10 months ago

Sort of.

In my use case when a user navigates from one entity to another, if there are different layouts for those entities then I want the active tab to be set to about on navigation. That property enables that behaviour. Setting it to false means that the active tab doesn't change when the user navigates to another entity which I understand to be the behaviour you want.

This revision to the profile layouts structure allows a profile author to create different layouts for different entity types. But in this revision (in contrast to how it was first implemented - the current production implementation) the layout doesn't need to be repeated for all of the possible combinations of entity type. The code will apply a layout when any of the entities in appliesTo match the entity type being displayed.

marcolarosa commented 10 months ago

Done

beepsoft commented 10 months ago

I just published the 0.40.1 version of the React component: https://www.npmjs.com/package/@describo/crate-builder-component-react.

This now includes the new vue integration without the web component.

marcolarosa commented 10 months ago

Thanks @beepsoft. I'll get onto #63 and #64 in the next week and I'll remove the web component stuff as well