OrchardCore
OrchardCore copied to clipboard
OrchardCMS
Add additional text info to content picker field, so user can distigu…
Added culture and autoroute (if filled) to content item picker, so user can easily pick content items. Using only display name has become a problem for multilingual sites with page named e.g. "blog" in multiple languages.
@deanmarcussen
I am not sure, if I should assign somebody to look at this, or what is the exact process with PRs? :)
@sebastienros
Ok, thanks for reply. So everything should always start with issue/feature request? I have created it backwards for now #8374 And the triage on Thursday is some public event, so I could possibly connect?
From your comment I see the issue, that not everyone would like this format of display, I think it can use similiar setup as Autoroute part => liquid syntax so you can define anything you want here, but I am not sure, if it is possible within contentpicker context.
@deanmarcussen
I have finally looked at aspect system and reworked this to aspects. Hopefully it is ok now - tested and still works, removed bunch of unnecessary code in existing implementations.
2 problems I met and not sure they are solved right: a) in ContentPickerAspect there are content part settings passed as object, because there can't be reference to ContentPickerFieldSettings - did not want to move settings or aspect, as aspect should be referenced from anywhere. b) in ContentPickerAspect - passing CultureInfo - > I cannot use ContentManager in ContentHandlerBase as it is circular reference, so it is passed into aspect, is it ok?
@deanmarcussen does it give sense to you like that? there are some merge conflicts now but I can update this branch with master if its ok with you
Concept looks good @MikeKry. Would suggest reading the previous pr review
@deanmarcussen
I looked at your comments when I reworked this and I think I have reflected all your comments in https://github.com/OrchardCMS/OrchardCore/pull/8280/commits/72661068bc33f4788ed78d478b54f91bc543beb6
I have gone through them again and I think everything should be ok, so I commented and resolved them.
We broke your Pull Request when we merged the Bootstrap 5 Pull Request. We may need to merge the latest main branch and fix the conflicts one by one on this PR.
@Skrypt Ok, I was out for a while, but I have merged it now and tested - still working ok.
@Skrypt
Just a question - does "Needs triage" mean anything for me, or is it just waiting for some group review? I am not very familliar with how this triage etc. works.
Needs triage means that we need to assign it to a milestone from a Thursday triage meeting. I use also this to make it prioritary over other PRs so that we can review it. As you can see here, no one approved it yet. So, even if I approve it then I need at least @sebastienros to approve it so that we can merge it. So, basically, this is a PR that can potentially also make it to the next release.
@Skrypt
Implemented suggested changes,
Merged main into this branch but I still need to check tomorrow if it did not break anything - kinda lot changes
@Skrypt
Tested, fixed codemirror. It is working fine again with the new changes. Maybe it should be reviewed by @MikeAlhayek, because there were many a lot of conflicts with https://github.com/OrchardCMS/OrchardCore/pull/12103
Sorry for the late reply, and for what's coming ;)
I think it can be much simpler with a single setting that contains the liquid template to use for the description of the content item in this list. Similarly to autoroute paths, with a default value that is just {{ ContentItem | display_text }}. Then users can customize each field to display the culture, type, ... without requiring any custom code. You did that part in this PR. I don't think we need to be able to do it by code.
NB: the way you were passing settings and culture to the aspect population made me realize this. Actually each implementation of the handler to populate the aspect should have resolved the culture if they needed it. Same for the settings.
@sebastienros
Not sure what you mean, because Autoroute paths generation also pass culture aspect to liquid templates (and imho it can't be generated without manually getting culture aspect).
It can be seen here: https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Modules/OrchardCore.Autoroute/Handlers/AutoroutePartHandler.cs#L406
Also there is no custom code needed from developer, title and description can resolve any liquid expression same as autoroute part.
But it is possible I misunderstood what you mean..? Maybe I would need a little more explanation.
Correct, you will need to get the culture from the aspect, but then the template is sufficient, something like this:
https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Modules/OrchardCore.Autoroute/Handlers/AutoroutePartHandler.cs#L400-L409
@sebastienros
So you mean, that new ContentPickerAspect is not needed? That is ok and I can change it, but feels like step-back to state before this commit https://github.com/OrchardCMS/OrchardCore/pull/8280/commits/72661068bc33f4788ed78d478b54f91bc543beb6#diff-c12e6f2791029f70fa686c607691139163bfd007933317dfb86d22c3122e671f
Where @deanmarcussen suggested, that it should rather be an aspect. Are you sure that we want to change it back? I have no problem in doing it, but I admit that I do not understand this concept of aspects and when to use them in OC :).
If so, please suggest me where to place function for resolving liquid expression for description/title. I am not sure whether it should be placed in some handler or service.
@sebastienros
What is your decision, should I revert it or no? I would like to finish this as it is open for 1 year already :)
@MikeAlhayek
Sure, I pushed merge into my branch. Is there possibility, that it will be merged any time soon? It starts to be little uncomfortable using content picker with growing number of content items.
I see that release 1.6 is already merged, but this PR did not make it through.. will it ever be merged?
Tasks to unblock the PR
- remove the aspect
- add a setting that defines the text to display in the list
- either use an extra checkbox to define an optional setting that will then be used
- use the setting all the time, and the default value is the liquid text to get the display name of the content item (backward compatibility)
I would go with a single field right now, not changing how it looks in the UI. We can always add an extra one later on if someone finds a reason to. The view model should not change then, and the view either.
Tasks to unblock the PR
* remove the aspect * add a setting that defines the text to display in the list * either use an extra checkbox to define an optional setting that will then be used * use the setting all the time, and the default value is the liquid text to get the display name of the content item (backward compatibility)
@MikeKry are you able to do this so it can be unblocked and hopefully merged before 1.7 is released?
@sebastienros @MikeAlhayek
Ok, I have removed aspect and field for description. I have added default value for backwards compatibility.
@MikeAlhayek
no, html is not rendered. I don't think, it would be good to allow html inside selector, it could break html structure etc.
All but one suggestions were applied to code in new commit.
@MikeAlhayek I think allowing HTML will solve for other scenarios. and if you are providing a custom template contained HTML, you should know what you are doing and the app should allow it.
If you don't mind, please add HTML support in the selector.
@MikeAlhayek
Okay, would you like html to be rendered both while selecting and also when items are already selected?
In example with hyperlinks it can cause redirection when clicking on item to select (but that may be also somehow controlled by html that is inserted)