OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.45k stars 2.41k forks source link

Multiple Fields Design Issue (Flaw maybe) #2480

Open JoshTango opened 6 years ago

JoshTango commented 6 years ago

This issue is regarding the fields feature when defining a content type. I will come at this from the perspective of the content picker, but it applies to all fields I suspect.

I have been researching other CMS platforms like Drupal 8 for example. In Drupal 8 you can decide 1 or a specific number or unlimited when you add a field. See the attached diagram. If you choose more than one you are presented with an add another item button as shown in the other attached diagram. This gives every field the ability to act like a bag/list/collection or whatever you want to call it.

I think you have a design flaw possibly. I think you should remove the multiple entity ability in the content picker for beta 3 release. Only have it pick one entity only for now. In time you should implement a 1 or specific number or unlimited ability for ALL fields not just the content picker. This should be asked for when on the screen when you first create the field. You can code this generically for all of them. You can create one UI for all field types. It would present just a list of title/value of the field and allow you to order them by moving the rows up or down like the UI you use for Bag right now.

Now on another related topic out of scope of this issue is the template for each field. In Drupal 8 they call it a teaser. It’s a classic Listview demo scenario where you have a smaller template of the entity in the listview and when you click it you go to the details page of the entity. Drupal allows you to pick which fields of the entity show up in the listview template/teaser and which ones show up in the detail page. For example, you might want to pick the short description for the list view template and pick long description for the detail page.

addanother unlimited
sebastienros commented 6 years ago

We are very well aware of these features, and have talked many times about such things. I would love to have you join our meetings to talk more in depth about these options and their implementation.

For instance we mentioned a few months ago that we might not need the multi-value fields if we supported fields cardinality. But there are some issues and also decisions to take, like how to implement that, how to store the values, and sometime the UI is better suited for more specific list selection than one by one selection.

About the teasers, we already want to make the display configurable. We did it for editors, we need to add "display modes" but it would be a the field level. Any conversation about it is a good one to have. Feel free to engage on gitter first, we can follow up on a call if you are willing to.

JoshTango commented 6 years ago

Drupal Views is your version of Lists. I think you should keep Lists as a query and not allow one by one selection. This is only if you do create a feature where one by one selection can be achieved a different way for those people that need that. As I get deeper into my analysis I may join in conversations.

sebastienros commented 6 years ago

Drupal Views is definitely not Lists. I implemented drupal views in O1 (called Projections) and Lists in both O1 and OC, trust me it's totally unrelated.

JoshTango commented 6 years ago

ok

JoshTango commented 6 years ago

I think I read that you are storing an array of Id's if they choose multiple content items. This is maybe fine for the implementation if you do it for all fields. So maybe the content picker multiple feature can stay as it is for now.

Skrypt commented 6 years ago

The feature that could be interesting is limiting the number of items that the content picker would allow. Though you could easily restrain the amount of item on your frontend afterward (Liquid). Is there any potential use case that would require this then?

sebastienros commented 6 years ago

Back to field cardinality. Once caveat is that changing the cardinality dynamically could change how we access the field values: Color.Value would become Color[0].Value. This could break some templates. But it's not worse than change the type completely.

Totally doable, without any big issues actually, we just need the default field setting editor to have an option to change the cardinality, from single to custom, which would allow for optional, or multiple items, with a minimum of maximum of instances. If the single option is selected then we can keep using the Color.Value pattern, otherwise we would use an array of fields. The editor should support ordering.

Technically it would duplicate what BagPart is doing to pre-render the scripts, and then to add an editor dynamically. This could be implemented any time as it's not a breaking change. Just a new property on the field definition.

sebastienros commented 6 years ago

/cc @douwinga @matiasmolleja

JoshTango commented 6 years ago

Well it would depend on where you accessed it. In a template typically it would be Color[I].value if it were in a loop to display. If you defined the template per field and saved that in the database it would not matter I don't think. The issue is internally in c# code not knowing if a object is going to be a collection or not. Perhaps the property has some code in it that retrieves the value of [0] if someone types Color.Value. So every field is an array but if someone is lazy and they just type Color.Value and dont specify square brackets then you just interrupt that as position 0;

Skrypt commented 6 years ago

Yeah, we need to predefine which value will be the default one in case that someone would remove the cardinality option on the field since the values stored would be different. Would we fallback to the position 0 in the JSON array or we define 2 different sets of JSON objects to persist values if we want to switch from one option to the other?

JoshTango commented 6 years ago

Also if someone switches back to 1 field only after having chosen multiple you should display a warning on the UI explaining the data will be lost. This is assuming you delete the data or maybe just leave it in the database?

Skrypt commented 6 years ago

I'd prefer leaving it in the database and use 2 different JSON objects for that matter. I was doing something like it with the Textfield predefined list when we decided it would never be a multi-value field. I think that what we we're doing in 01.

sebastienros commented 6 years ago

An option is to always store the first value as Color.Value

Skrypt commented 6 years ago

Yes but if you switch from one option to the other then you would lose persistence of the user previous choice.

JoshTango commented 6 years ago

Here is another perspective. We are thinking inside the box rather than outside the box. If this was a normal .Net project we would have object and List. So ContentType should have List or FieldTypePart based on what the user selected. We are trying to think how to implement a list feature inside of an element of the list which is weird. Now in the UI we can present it like that just like Drupal 8. But in the implementation I think we should have object or List. Just an idea. This way the fieldpartcode can stay the same and you would have to alter the content definition and content code I assume.

JoshTango commented 6 years ago

I would peek inside drupal code and see how they approach it maybe?

Skrypt commented 6 years ago

Yes I agree but the data saved needs to be consequent to the fact that the field can be a List or an Object too. That's all I'm saying. If we have a IsMultiValue data saved on the Content Type then we can evaluate which kind of data will be returned.