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 #6595

Closed
wants to merge 4 commits into from
Closed

Placements module #6595

wants to merge 4 commits into from

Conversation

TFleury
Copy link
Contributor

@TFleury TFleury commented Jul 6, 2020

Related to #6503

The Placements module add an optional feature that lets user define custom placements rules.
It adds a placement.json file stored at site's root and an admin UI to edit placement rules.

Todos :

  • Add DeploymentSteps

Captures :
List
Edit

TFleury added 3 commits July 6, 2020 18:06
Move PlacementNode processing to a distinct service, so it can be used outside of ShapePlacementParsingStrategy
@ns8482e
Copy link
Contributor

ns8482e commented Jul 6, 2020

Nice, However IMO if we are considering placement in Admin UI then, it should be nice to be organized by Content Type and not by Parts/Shapes. i.e. Instead of Generic Placement menu for all Parts/Shapes in the system - there should be "Placement" tab in Edit Content Type that would show only relevant Parts/Shapes to configure placement for that content type only.

@TFleury
Copy link
Contributor Author

TFleury commented Jul 8, 2020

@ns8482e that was my first idea (I tried to add placement settings to content parts/fields definition), but there are more shapes than just default ones, and this may require to repeat rules across content types.
Placement file is the most powerfull and flexible model, it's already documented and filters are extensible.
But I think we could add shortcuts in content definition UI, like in Templates module.

@deanmarcussen
Copy link
Member

@TFleury @ns8482e it would be nice to see shortcuts, but is another issue - feel free to work on something :).

It's related to shapetracing #4501.

You can't easily add shortcuts directly in the content definition UI, as many alternates, and information that is relevant about placement is only available when the display is rendered. The data is just not available if you don't call BuildDisplay, and even then it is not readily available in theadmin, as it will be affected by what templates do in the theme.

One idea we had around shape tracing is to have a developer app, much like the vuejs developer tools, or augury for angular, if you are familiar with either?

Seb wants to see it on the front end, I'd quite like to be able to see it in content definitions area, as well (for me that would be really useful). One possibility for achieving both was to reuse much of the functionality from the content preview feature to render the content item in a way that a provided the appropriate shape meta data inside the live preview, to a developer tool.

@TFleury I haven't had time to have a good look at this pr, but two things pop to mind

  • Why did you choose to store them in a file system, rather than the database? (difficult for distributed)
  • I saw you have removed the ConcurrentDictionary. That was important for building multiple shape tables concurrently
  • How does the shape table cache get cleared when changing alterations?

@TFleury
Copy link
Contributor Author

TFleury commented Jul 8, 2020

@deanmarcussen Thank you for the quick review.

Why did you choose to store them in a file system, rather than the database? (difficult for distributed)

I found it consistent to have it near the ContentDefinition.json, however there is an abstraction on that (IPlacementFileStore) and it could be easily moved to database (optionaly or by default)

I saw you have removed the ConcurrentDictionary. That was important for building multiple shape tables concurrently

Maybe I missed something, but I didn't see any concurrency issue here.
Before the PR we passed the global _shapeDescriptors dictionary (and there could have been concurrency issues), but with the PR we provide activeFeatureDescriptors dictionary which is distinct for each shape table build.

How does the shape table cache get cleared when changing alterations?

I added ITenantShapeTableProvider which inherits from IShapeTableProvider and have a ChangeToken.
Alterations provided by ITenantShapeTableProvider are not globaly cached and the change token raises expiration of all related shape table cached entries.

@TFleury
Copy link
Contributor Author

TFleury commented Jul 8, 2020

I think the most important thing is to allow per tenant shape table providers with an expiration mechanism.

Afterward, we will be able to provide different ways to add shape table alterations.

I can make a distinct PR with the two firsts commits only, and then we can discuss about the best way to provide alterations at runtime in the admin UI.

@deanmarcussen
Copy link
Member

Thinking about this, I would have thought that this module is much more like the Templates module, which doesn't trouble the shape table at all, but uses a binding resolver.

It's usage is also pretty similar, i.e. people that are developing templates through the UI. (I think?)

We should get some consensus on that before asking you to change it though.

I found it consistent to have it near the ContentDefinition.json, however there is an abstraction on that (IPlacementFileStore) and it could be easily moved to database (optionaly or by default)

The content definition file store is actually a feature (on by default), where the database store is more recommended for production.

This would be good to see a demo at during the Tuesday meeting

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.

3 participants