Refactor media shapes and correct formatting in views #6676
Conversation
There was a problem hiding this comment.
@PiemP some clean up (I should have caught it better on the review)
You just need to be careful when you are reformatting views as well.
I find it helps to look at the files changed in github here after submitting them, and then when you see you have added something that is unecesary it is easier to remove.
| @@ -38,7 +37,6 @@ | |||
| using OrchardCore.Media.Shortcodes; | |||
| using OrchardCore.Media.Services; | |||
| using OrchardCore.Media.Settings; | |||
| using OrchardCore.Media.Shape; | |||
There was a problem hiding this comment.
removed extra usings added for no need
|
|
||
| <script asp-src="~/OrchardCore.Media/Scripts/media.js" asp-name="media" at="Foot" depends-on="admin, vuejs, sortable, vuedraggable"></script> | ||
| <style asp-src="~/OrchardCore.Media/Styles/media.min.css" debug-src="~/OrchardCore.Media/Styles/media.css"></style> |
There was a problem hiding this comment.
returned this to the view it belongs to.
the resource manager handles multiple script inclusions by design, so you don't need to add a wrapper to get this on the page only once.
| @@ -3,54 +3,52 @@ | |||
| @using OrchardCore.ContentManagement.Metadata.Models | |||
|
|
|||
| @{ | |||
| var settings = Model.PartFieldDefinition.GetSettings<MediaFieldSettings>(); | |||
| var settings = Model.PartFieldDefinition.GetSettings<MediaFieldSettings>(); | |||
There was a problem hiding this comment.
returned all the formatting in this view to good styling
| @@ -0,0 +1,12 @@ | |||
| @if (!Context.Items.ContainsKey("MediaFieldEditLocalization")) | |||
| { | |||
| @* Insert only once per request *@ | |||
There was a problem hiding this comment.
this was all you needed to do to resolve the issue with multiple ids in the dom.
We use the shapes technique in the other modules, so that the media feature can be disabled, and the shapes will not load a modal for the media app (which is disabled)
What you did here was add a shape wrapper in the media module, which required that the media module was active (so was unecesary)
|
understand... thanks @deanmarcussen... more easy to do than to explain |
|
Thanks. So we didn't need custom shapes, my assumptions were right? |
Yes, the assumptions are correct. We still need all the shape wrappers for the That being said, it would be good to rationalize some of these models / trumbowyg scripts at some point, when I have time, as there is much duplicate code between them, creating confusion, and extra checks in scripts, for no particular need. Using the wrappers there also defeats the purpose of being able to easily add an editor more, because you also need to add a shape wrapper to make it work. There is probably a simpler way, but for another pr |
Fixes issues with #6172
/cc @PiemP