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

pat-contentbrowser component registry #1396

Merged
merged 7 commits into from
Dec 18, 2024

Conversation

petschki
Copy link
Member

This registers SelectedItem component with @plone/registry to make it overridable.

@petschki petschki force-pushed the petschki-contentbrowser-componentregistry branch from 86945ac to 4e5693c Compare September 27, 2024 07:05
@petschki petschki marked this pull request as draft October 2, 2024 08:34
@petschki petschki force-pushed the petschki-contentbrowser-componentregistry branch 2 times, most recently from 915762f to 1594b8b Compare November 30, 2024 19:21
@petschki
Copy link
Member Author

petschki commented Dec 3, 2024

This solution provides a general approach to override components with @plone/registry but we also should provide a way, so that an integrator is able to do this only for specific widges. In relateditems it was possible with pattern_options which could be overriden with autoform schemahints, but here I do not (yet) have an idea how to bring this together with @plone/registry ...

@MrTango @thet you might have an idea on this?

@MrTango
Copy link
Contributor

MrTango commented Dec 3, 2024

Can you please explain a bit more how this was done with pattern_options?
One idea would be to also provide a property via pattern_options which is used inside the pattern to make a switch to something else then the default. Like another name for a component. This switch would be only used for that widget.

@MrTango
Copy link
Contributor

MrTango commented Dec 3, 2024

So if we combine the lookup from the registry, with pattern_options for each component we want to make customizable, we can change anything, even on per field level.

For example, if we want this for SelectedItem, the default name for the component is pat-contentbrowser.SelectedItem, but via a pattern_option selected_item_component we could say that we want pat-contentbrowser.SelectedItemAdvanced to be used and register it also in the registry. And even that, one could override later in a different package via registry.

@petschki
Copy link
Member Author

petschki commented Dec 3, 2024

Historical: you could provide either a selectionTemplateSelector option, which had a CSS selector to a html snippet or you put the full template code (underscore XML template) inline with selectionTempate option. This is then applied with the formatSelection method from select2 https://github.com/plone/mockup/blob/master/src/pat/relateditems/relateditems.js#L531 ... and there were more templates to override: https://github.com/plone/mockup/blob/master/src/pat/relateditems/relateditems.js#L507-L513 ...

I like the idea of providing custom registry keys to the pattern options. I could think of a dictionary, since there could be more overridable components in the future:

data-pat-contentbrowser='{
    "components": {
        "selectedItem": "pat-contentbrowser.MyAdvancedSelectedItem",
        "browserItemPreview": "xyz",
        ...
}'

@thet
Copy link
Member

thet commented Dec 3, 2024

@petschki this is an interesting approach. But I think this misses the use case a bit.

With the previous pat-relateditems approach you could change the selected item template per related items widget instance. Now you can only change the component for all reference browser instances.

What I think would be really useful is to use a different selected item component based on the context. As equivalent in Plone: a different browser view based on the implemented interface of the context.

It looks like that the @plone/registry does support this via the dependencies option (they should have called it context or contexts):

https://plone-registry.readthedocs.io/how-to-guides/register-and-retrieve-components.html

thet
thet previously requested changes Dec 3, 2024
Copy link
Member

@thet thet left a comment

Choose a reason for hiding this comment

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

See my comment here: #1396 (comment)

@@ -85,6 +92,10 @@
selectedItemsNode.dispatchEvent(events.change_event());
}

function LoadSelectedItemComponent(node, props) {
const component = new RegisteredSelectedItem.component({target: node, props: props});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const component = new RegisteredSelectedItem.component({target: node, props: props});
const component = plone_registry.getComponent({
name: "pat-contentbrowser.SelectedItem",
dependencies: [props.item["@type"]]
}).component;
return component({target: node, props: props});

NOTE: untested pseudo code

Comment on lines 24 to 30
// get selectedItem component from registry
const RegisteredSelectedItem = plone_registry.getComponent("pat-contentbrowser.SelectedItem");

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// get selectedItem component from registry
const RegisteredSelectedItem = plone_registry.getComponent("pat-contentbrowser.SelectedItem");

Replaced by below...

@petschki
Copy link
Member Author

petschki commented Dec 3, 2024

I talked with Victor about that approach and it also seems to be a solution. But your suggested code provides the type of an item, this would mean, that for each item type the selected Item could be different. Hm ... not sure if someone wants that. The usecase (like it was in relateditems) is to provide a different selecteditems list "per widget", so for example, we have this addon which has "related_images" where we want to list the selected images as a 3-colum card list with input fields for editing convenience. This is how it looks and what we achieved using selectionTemplate option from pat-relateditems.

Bildschirmfoto 2024-12-03 um 14 30 08

This would mean we could maybe use a combination of the contexttype.fieldname as dependencies then we can provide both ... per context and per field. Would further be nice to have something like image.* to override the image type for all widgets... or does the dependencies list maybe handle this?

@thet
Copy link
Member

thet commented Dec 3, 2024

According to the @plone/registry docs the dependencies list allows to define one component for multiple dependencies.
I don't know more about the details, but the example shows that @type is used.

I'd be OK having a per-type lookup - as long as it doesn't add a significant performance impact. It would be another improvement over the previous situation. That way you could for example render the selected items for different content types differently - an image has different layout requirements than a news item or a location venue or a event.

@MrTango
Copy link
Contributor

MrTango commented Dec 3, 2024

I prefer the version where we control it in the widget, then we can have multiple veriantion in the same content type for different fields. @thet the registry like we use it now is globally. But if we allow chnaging the name of the component to use via patterns_options and push this into the svelte app via props, we are very flexible on per widget level.
I don't know how the dependency parametr in plone/registry works, but it sounds like something different than what we want, if it's not on field/widget level.

@MrTango
Copy link
Contributor

MrTango commented Dec 3, 2024

So to register a new variant we use plone/registry, this variant can be registered under one of the default component names and will override them. Or it can be registered as a new component under a different name and can then be used on per widget level per patterns_options. If the dependency function works like you thing, we can implement this too. So by registering a new component under default name, but with a pendency on a content type, all relation fields would use this implementation. But still allowing it to be overridden by pattern_options per widget.

@petschki
Copy link
Member Author

petschki commented Dec 4, 2024

I followed the idea of @MrTango and added a new parameter componentRegistryKeys bbbf59d

This parameter (a dictionary to make other components also customizable in the future) can be customized with pattern_options to provide a custom key with a customized component. I've implemented this in an addon from us:

Custom Component: collective/collective.behavior.relatedmedia@8ddfaae
Pattern options: collective/collective.behavior.relatedmedia@8e168ab

And it works 🎉

@thet the dependencies approach could be additionally implemented to further customize the selected item on "per type". But that is out of scope of this PR I think.

@petschki petschki marked this pull request as ready for review December 4, 2024 11:21
@petschki petschki force-pushed the petschki-contentbrowser-componentregistry branch from 5cf4a2d to 2dcea4d Compare December 17, 2024 12:50
@petschki petschki requested a review from thet December 17, 2024 13:51
@petschki petschki force-pushed the petschki-contentbrowser-componentregistry branch from abcf5d3 to 7c6eca4 Compare December 18, 2024 07:11
@petschki petschki dismissed thet’s stale review December 18, 2024 08:09

registry dependencies is not the right approach in this case... solved it with custom registry keys.

@petschki petschki merged commit 9906aff into master Dec 18, 2024
3 checks passed
@petschki petschki deleted the petschki-contentbrowser-componentregistry branch December 18, 2024 08:09
class="pat-contentbrowser"
data-pat-contentbrowser='{
"customComponentKeys": {
"SelectedItem": "pat-contentbrowser.myfield.MySelectedItemComponent",
Copy link
Member

@thet thet Dec 23, 2024

Choose a reason for hiding this comment

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

@petschki @MrTango Regarding the custom component keys - isn't selectedItem the only component key which is supported yet?
I'm a bit restrained to use JSON structures as pattern options - especially JSON objects. Until now I had the impression that JSON objects prevent us from using the Patterns options parser and thus prevent us currently from using the BasePattern (that can be fixed).
But your PR here shows that this is not entirely true - You're using both the Patterns Parser and BasePattern here. However, at least the customComponentKeys is currently not formally defined in the Patterns Parser.
I'd have maybe made a flat single option out for the selectedItem component. Something like selectedItemComponent. Then we can use Patternslib CSS style Pattern option definition like data-pat-contentbrowser="selectedItemComponent: my-component;".

But since this seems to work fine, all is good!

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