Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Placements module v2 #7483

Merged
merged 14 commits into from
Dec 5, 2020
Merged

Placements module v2 #7483

merged 14 commits into from
Dec 5, 2020

Conversation

TFleury
Copy link
Contributor

@TFleury TFleury commented Nov 2, 2020

Fixes #6503

The Placements module add an optional feature that lets user define custom placements rules in admin UI.

This PR is a rewrite of #6595, it uses IShapePlacementProvider introduced by #6780 to provide placements at runtime.

  • Placements are stored in DB
  • Supports Deployment and Recipies
  • Adds shortuts to content type editors

Captures :

1-PlacementsList
2-PlacementEditor
3-ContentTypeShortcut
Shortcuts zone :
4-ShortcutsZone

Copy link
Member

@deanmarcussen deanmarcussen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it.

When is the demo? :)

Couple of minor comments.

I know I asked you to move it to the database last time, which is great, thank you.

For the future we could also have the file option (as a feature) - both have good uses.

@jptissot
Copy link
Member

jptissot commented Nov 3, 2020

This might be too much work / impossible but could we add hints to the different placement options using the ContentDefinitionManager ? For example, instead of having a "placement rule" code mirror field, could we generate inputs / dropdowns, etc for each of the placement options ?

@TFleury
Copy link
Contributor Author

TFleury commented Nov 3, 2020

@deanmarcussen thanks for the review.

For the future we could also have the file option (as a feature) - both have good uses.

Of course, I will add an abstraction of the storage.

This might be too much work / impossible but could we add hints to the different placement options using the ContentDefinitionManager ? For example, instead of having a "placement rule" code mirror field, could we generate inputs / dropdowns, etc for each of the placement options ?

It could be possible, but placement node format is extensible (with IPlacementNodeFilterProvider) so we can't have a fixed list of fields/properties, or we will prevent usage of extensions. Furthermore, it's a developer feature, the idea is to be consistent with Placement.json. (maybe I should add link that points to placement documentation 🤔)

@jtkech
Copy link
Member

jtkech commented Nov 3, 2020

For info we already have an implementation of a generic IDocumentManager<TDocumentStore, TDocument>

/// <summary>
/// An <see cref="IDocumentManager{TDocument}"/> using a given type of <see cref="IDocumentStore"/>.
/// </summary>
public interface IDocumentManager<TDocumentStore, TDocument> : IDocumentManager<TDocument>
    where TDocumentStore : IDocumentStore where TDocument : class, IDocument, new()
{
}

And an implementation of IFileDocumentStore

/// <summary>
/// A singleton service using the file system to store document files under the tenant folder, and that is in sync
/// with the ambient transaction, any file is updated after a successful <see cref="IDocumentStore.CommitAsync"/>.
/// </summary>
public interface IFileDocumentStore : IDocumentStore
{
}

Allowing to just inject

IDocumentManager<IFileDocumentStore, PlacementsDocument>

@scleaver
Copy link
Contributor

scleaver commented Nov 3, 2020

What's the priority of conflicting placements with this feature? Say I have placement in my theme for FooField, then there is one in the feature FooField module, and then another in the UI for FooField with the same shape differentiator etc but a different place rule for example.

@sebastienros
Copy link
Member

I don't like shapes, or placement (on the front-end), but I am ok with this kind of PR.

@TFleury
Copy link
Contributor Author

TFleury commented Nov 4, 2020

@deanmarcussen For the future we could also have the file option (as a feature) - both have good uses.

Done !

@scleaver What's the priority of conflicting placements with this feature?

PlacementInfo are combined (see PlacementInfoExtensions.cs).
Placements module has a higher priority. It will override Location and ShapeType (if defined), and add Wrappers and Alternates.

@ns8482e
Copy link
Contributor

ns8482e commented Nov 4, 2020

Placements are closely related to Display and not Content - So It would be nice to refactor the module into Abstractions, Core, Module to support OrchardCoreFramework not just as CMS module

Copy link
Member

@deanmarcussen deanmarcussen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, really just a bit of formatting, and conventions to do.

(Plus merge dev - conflicts)

Nice job on adding the file store too

src/OrchardCore.Modules/OrchardCore.Placements/Startup.cs Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.Placements/Startup.cs Outdated Show resolved Hide resolved
<a asp-route-action="Edit" asp-route-area="OrchardCore.Placements" asp-route-shapeType="@Model.ShapeType" asp-route-returnUrl="@FullRequestPath" class="btn btn-primary btn-sm" role="button">@T["Edit"]</a>
<a asp-route-action="Delete" asp-route-area="OrchardCore.Placements" asp-route-shapeType="@Model.ShapeType" class="btn btn-danger btn-sm" role="button" itemprop="UnsafeUrl RemoveUrl">@T["Delete"]</a>
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newline

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't edit this repo, so can you add the newline here please

</div>
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newline

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't edit this repo, so can you add the newline here too please

@deanmarcussen
Copy link
Member

@ns8482e I'm not sure how this would benefit from a core, or abstractions project?

It uses the extension interface already added by @TFleury from DisplayManagement.

The abstractions and core projects are by and large more about other modules being able to safely reference each other.

I can't think what you would put from this in a core or abstractions projects, for any real benefit to another module?

Maybe I am wrong?

@ns8482e
Copy link
Contributor

ns8482e commented Nov 6, 2020

@deanmarcussen I mean this feature can be used in OrchardCoreFramework as well with organizing the code in abtractions/core.

I mean UI and CRUD will still be in the CMS module, However moving Document and FileStore related code to Core, and provide service extension eg. service.AddTenantPlacement() for framework that would allow developers to configure dynamic placement for OrchardCoreFramework without needing CMS module

Copy link
Member

@deanmarcussen deanmarcussen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @TFleury I didn't see you had updated this a while back.

@agriffard
Copy link
Member

@TFleury As it is a new module, can you please add the bare minimum docs: A specific folder and readme file in the src/docs/reference with a description of the module and add it to the TOC in mkdocs.yml.

@agriffard
Copy link
Member

Asking again: Can you please add some docs @TFleury before we can merge it?

@TFleury TFleury requested a review from agriffard as a code owner December 5, 2020 17:05
@TFleury
Copy link
Contributor Author

TFleury commented Dec 5, 2020

@agriffard thanks for the reminder, I just added it.

@agriffard agriffard merged commit bcdf173 into OrchardCMS:dev Dec 5, 2020
@agriffard
Copy link
Member

Thank you. 162nd OC project. 👏

@MikeKry
Copy link
Contributor

MikeKry commented Dec 8, 2020

@agriffard

Just tested this, but I can't figure out how to actually make this work for admin placements?
Figured out that when I create custom placement (from /Admin/Placements) with "_Edit" suffix, it works. But does not work for content parts created in administration and I have not found a way how to edit admin layout from content type detail via edit placements btn.

for example:
i would like to move custom part to another tab:
image

only way to do that is per single field (tried everything else and nothing works)
image

and I have to create in listing, because it does not work if created by button in part definition
image

because it does not have _edit suffix

@TFleury
Copy link
Contributor Author

TFleury commented Dec 8, 2020

You can try this :
SeoPlacement

@MikeKry
Copy link
Contributor

MikeKry commented Dec 8, 2020

@TFleury
That works half-way, SeoPart text fields are displayed in separate tab, but also all other text fields are moved to that tab. Seems like it means, that all text fields on page with "SeoPart" are displayed in that tab.

Could be nice if placement docs would be updated :D

I think that I tried every possible combination.. maybe it just does not work on custom parts?
But it should work with parts created in code, implementing ContentPartDisplayDriver..

Imho it is needed to allow placement of custom parts defined in admin, also it would be great if differentiator would work only by fieldname without contentpart prefix

@giannik
Copy link
Contributor

giannik commented Dec 13, 2020

related maybe #7952

@PBMikeW
Copy link
Contributor

PBMikeW commented Aug 26, 2021

I had the exact same issue as MikeKry

@ns8482e
Copy link
Contributor

ns8482e commented Aug 26, 2021

@PBMikeW there has been PR #8935 to solve this - However it is stalled now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature : Define placement in admin