Implement missing forms selectpart and options

[deanmarcussen]

Fixes #3297
Fixes #4094

Similar ui to the predefined text field list settings.

I chose not to do it with SelectListItem and / or optgroup to keep everything simple, and consistent with the rest of the forms module.

[ns8482e]

@deanmarcussen I have started working on this too.

Can you add my request here? Support for Type, placeholder and optgroup?

e.g. type being Dropdown list, radio list, List box, checkbox list?

[deanmarcussen]

Can you add my request here? Support for Type, placeholder and optgroup?

e.g. type being Dropdown list, radio list, List box, checkbox list?

I thought about those, but I saw this as a SelectPart and would see a radio group as a different part.

Maybe not?

it could be extended for optgroup, but I had to do this for a client, and wanted to keep it simple, so thought I'd push it up after.

Feel free to take over the pr, I am short of time to do anything much more to it

[ns8482e]

@deanmarcussen No worries, I can add my PR on top of your feature.

I thought about those, but I saw this as a SelectPart and would see a radio group as a different part.

Maybe not?

Technically if we map SelectPart to <select> tag than it seems different. But when we look at from functional side - it seems same for form designer perspective as he could change display seamlessly for single select - from dropdown to radio list and for multi select listbox to checkbox list without reworking on form design for same data -

[deanmarcussen]

Technically if we map SelectPart to <select> tag than it seems different.

Yes, I essentially agree, but currently the form part's are matched to a tag type.

i.e. the input part supports radio buttons etc.

Hence why I felt we should probably continue that.

Otherwise we're changing our minds partway through, and they become a strange mix.

[deanmarcussen]

Might be worth asking @sfmskywalker what the original design intentions of the forms module was?

[ns8482e]

i.e. the input part supports radio buttons etc.

OK, But IMO - using radio/checkbox with input and making list of choices and maintaining ModelState on postback requires some sort of html asp.net knowledge :).

It's kinda too much expectation from non technical form designer.

[ns8482e]

@deanmarcussen anyway continue with your PR, I'll create PR for my request.

[jptissot]

Why not just add a bag of SelectItems / OptGroups ?

[Skrypt]

I like that Vue.js component I wonder who did it? 🙈
Just a small comment here. We need to add a margin between those form elements. They all look stacked on top of each others. No clear different margin between a title and the previous elements group. It needs to breathe a little !!!

@ns8482e Is there a functional reason why you added this style?

OrchardCore/src/OrchardCore.Modules/OrchardCore.Flows/Assets/scss/flows.edit.scss

Lines 4 to 6 in 583467a

	div.form-group {
	margin-bottom: 0px !important;
	}

See #7292 for fix.

[Skrypt]

Yeah it could definitely use a SelectListItem / OptGroups but I believe the Vue.js component doesn't do it for now. We'd need to be able to add OptGroups when we click on the small edit icon and then display a list of those OptGroups as a new column in the Option items. I think it's fine enough for now while we wait on someone to work on adding this on the Vue.js component.

[Skrypt]

LGTM after small fixes

[ns8482e]

@ns8482e Is there a functional reason why you added this style?

To reduce white space - when used in smaller columns like 25%, 33%

[jptissot]

Yeah it could definitely use a SelectListItem / OptGroups but I believe the Vue.js component doesn't do it for now. We'd need to be able to add OptGroups when we click on the small edit icon and then display a list of those OptGroups as a new column in the Option items. I think it's fine enough for now while we wait on someone to work on adding this on the Vue.js component.

But why not simply use Widgets and a bag instead of writing a custom implementation ?

[Skrypt]

@jptissot That would be for listing ContentItems right? Then this would be more like a ContentPicker.

[jptissot]

No, I mean the Select has a Bag (named Options) of SelectItem / SelectOptGroup and you build those as Content Items instead of adding a custom editor for Options.

[Skrypt]

Yes a content item picker... in then end what you get is a content item list right?

[deanmarcussen]

why not simply use Widgets and a bag instead of writing a custom implementation ?

Yes, I saw your comment on the issue @jptissot , that you had resolved it that way.

It tends to be the way I build sites as well, bags, bagcontainers, and bagitems, to create lists inside content.
But I tend to think that approach is more appropriate to theme development, not our internal module development.

The reason I didn't for this was because you do end up with content items, which makes the templating more complex, and the part becomes not hard typed.

i.e. you can't just have an array on it, it becomes an array of content items.

As I said, I'm reasonably easy on this pr, I needed it for a client, so figured I'd push it back to OC afterwards.

If you guys think it should be done differently, then I don't mind closing it, but the issues have sat there for a year and a half without anyone doing anything to fix it ;)

[ns8482e]

No, I mean the Select has a Bag (named Options) of SelectItem / SelectOptGroup and you build those as Content Items instead of adding a custom editor for Options.

Then it’s kinda data source to SelectPart

[Skrypt]

I think we should have both options. These are 2 different option lists which makes sense to have.

[Skrypt]

@jptissot Will it break your actual SelectPart because of naming? Maybe we could name this one as SimpleSelectPart and come up with a ContentItemSelectPart later on ...

[ns8482e]

@Skrypt SelectPart and Select content type metadata are already defined in Migration

[Skrypt]

Ok I understand now why we debate on the implementation.

OrchardCore/src/OrchardCore.Modules/OrchardCore.Forms/Migrations.cs

Lines 76 to 84 in 7de0dbe

	// Select
	_contentDefinitionManager.AlterPartDefinition("SelectPart", part => part
	.WithDescription("Provides select field properties."));
	_contentDefinitionManager.AlterTypeDefinition("Select", type => type
	.WithPart("FormInputElementPart")
	.WithPart("FormElementPart")
	.WithPart("SelectPart")
	.Stereotype("Widget"));

I think we can use the same metadata for every Driver we would implement here?

[ns8482e]

I guess yes, you can have multiple drivers contribute in rendering for the same Part.
See CommonPart for example.

[Skrypt]

So here what I'm saying is that we should probably decide if that's the name we want to use for the Basic implementation which uses the Vue.js viewcomponent.

[ns8482e]

You mean SelectPartDisplayDriver? Seems fine. For future enhancement - functionality can be added within that driver.
Also I guess it doesn't affect any custom modules, if it's using the same name for driver.

[Skrypt]

Ok so I'm leaving you decide if this PR needs more work or not. I'm fine with it. Better have something than nothing too.