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

Implement ContentBrowserWidget for the new pat-contentbrowser pattern #197

Merged
merged 5 commits into from
Sep 20, 2024

Conversation

petschki
Copy link
Member

@petschki petschki commented Mar 8, 2024

ContentBrowserWidget is simply a copy of RelatedItemsWidget and exposes the new pat-contentbrowser which has the same API and features as pat-relateditems right now.

The following core modules use RelatedItemsFieldWidget:

  • plone.app.relationfield.behavior
  • plone.app.multilingual.interfaces
  • plone.portlet.collection.collection
  • plone.volto.behavior.preview_link
  • plone.volto.coresandbox.example

For now we only update the relatedItems field in plone.app.relationfield. The others can be updated afterwards.

UPDATE No. 1: RelatedItemsWidget is now a pure alias of ContentBrowserWidget using zope.deferredimport

UPDATE No. 2: RelatedItemsWidget is not replaced! Everything works like before, only ContentBrowserWidget functionality is added. Nothing changes until plone.app.relationfield schema gets updated to the new widget (see PR below).

This PR should be tested with:

NOTE: robottests in Products.CMFPlone need to be updated too for the new contentbrowser markup.

@mister-roboto
Copy link

@petschki thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

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.

Some remarks in the code.

In general: Do we want to drop support for pat-relateditems completely? Then we can also remove the Pattern from Mockup.
But isn't that a breaking change? Or is the plan to provide a pat-relateditems drop-in replacement with a Pattern alias for pat-relateditems?

plone/app/z3cform/templates/contentbrowser_display.pt Outdated Show resolved Hide resolved
plone/app/z3cform/templates/contentbrowser_display.pt Outdated Show resolved Hide resolved
plone/app/z3cform/templates/contentbrowser_display.pt Outdated Show resolved Hide resolved
plone/app/z3cform/widgets.zcml Outdated Show resolved Hide resolved
@thet
Copy link
Member

thet commented Apr 3, 2024

Okay, checking one of my more complex projects, I'm not using any templates which would directly use a pat-relateditems class - It's only using it via plone.autoform.directives.widget configuring the RelatedItemsFieldWidget.
So the pat-relateditems alias is maybe not a big deal but maybe still useful.

@petschki
Copy link
Member Author

petschki commented Apr 4, 2024

Linking pat-relateditems internally to pat-contentbrowser seems like a nice idea. It would help moving away from patched select2 faster.

I started testing this implementation with example.contenttype and all the widgets are switched to contenbrowser. Though there are some tweaks for the vocabulary lookup like @@getSource doesn't work like expected. Doesn't look like it has something to do with this PR.

@MrTango
Copy link
Contributor

MrTango commented Apr 11, 2024

@thet the general idea is to provide it with plone.classicui it now as an alternative, already usable in Plone 6.1 and later drop the old implementation in Plone 7.
So if one wants, she can install plone.classicui and have this already and some other things in time.

@petschki
Copy link
Member Author

@MrTango meanwhile I'm against the "opt-in" approach in favor of automatically provide the new widget with zope.deferredimport like its done in this PR ... otherwise we cannot move on with removing the old relateditems pattern and its select2 implementation ... if it's that simple to remove RelatedItemsWidget without breaking something, I'd say: lets do it!

@MrTango
Copy link
Contributor

MrTango commented Apr 11, 2024

The only thing is, we are also having a different feature set and API of the pattern, which is a breaking change too.
I know we want to get rid of the old Select2 version and having this is Plone 6.x means a breaking change on the library level. If we do that, we should be very clear about the fact.

@petschki
Copy link
Member Author

ok... my understanding of pat-contentbrowser was always to be a feature complete replacement of pat-relateditems without any difference in the API but maybe some more handy features (like multiselect eg.)

the only thing which really would mean a breaking change (we talked about that yesterday) is the dropped "search mode" ... but to be honest, this could be implemented very simple in pat-contentbrowser right now with just one level an no path depth restriction.

@MrTango
Copy link
Contributor

MrTango commented Apr 11, 2024

ok than let's go in that direction, if we keep it non breaking, even better.

@petschki petschki force-pushed the pat-contentbrowser-widget branch 3 times, most recently from 58c3119 to c026425 Compare May 22, 2024 10:28
@petschki
Copy link
Member Author

@jenkins-plone-org please run jobs

@petschki
Copy link
Member Author

Latest changes, as talked to @thet lately:

  • RelatedItemsWidget is kept untouched for now
  • ContentBrowserWidget is added as feature and can be used as widgetFactory or plone.autoform schema hint.

As you can see, there's nothing breaking in the current coredev tests in jenkins ... though these changes will bump version to new feature release 4.7.0 which we then can depend on in other packages (like plone.app.relationfield) where we implement the pat-contentbrowser pattern later.

/cc @plone/classicui-team

@petschki petschki marked this pull request as ready for review May 22, 2024 12:21
@thet
Copy link
Member

thet commented May 22, 2024

@petschki your last comment reads like we're still keeping the related items pattern and only provide the content browser pattern for those who actively using it. Is this right?
The changelog suggest otherwise.

Can the PR plone/plone.app.relationfield#51 ignored for now?
Which are the PRs, which need to be reviewed and eventually merged together?

@petschki
Copy link
Member Author

petschki commented May 22, 2024

Well ... the relateditems pattern is used in several places spread all over the core. This widget here is for z3c.relationfield fields like plone.app.relationfield which can easily switch from RelatedItemsFieldWidget to ContentBrowserFieldWidget without changeing the options. But first of all, the mockup PR has to be merged and released in plone.staticresources before using pat-contentbrowser anywhere else of course.

I just found out, that the LinkWidget (used in Link dexterity contenttype) has a custom input template which uses pat-relateditems directly in the markup (see https://github.com/plone/plone.app.z3cform/blob/master/plone/app/z3cform/templates/link_input.pt#L28) ... this can also be changed here or in a separate PR.

And there is the integration in pat-tinymce (link plugin) and pat-upload (target selector) but this belongs to the mockup PR.

So there are the following steps to replace pat-relateditems. Order is relevant:

  1. merge pat-contentbrowser implementation mockup#1377 and release in plone.staticresources -> this delivers pat-contentbrowser and uses it in pat-tinymce and pat-upload
  2. merge this PR -> delivers z3c.form widget and uses pat-contentbrowser in LinkWidget
  3. merge Implement new ContentBrowserWidget plone.app.relationfield#51 -> this implements ContentBrowserWidget in relatedItems field
  4. Fix CMFPlone robottests -> the new contentbrowser UI has to be updated in the corresponding robottests ... I've started this already, PR coming soon.
  5. Update the other core packages mentioned in the description on top here.

UPDATE: these steps depend on the order but do not need to be made all at once.

plone/app/z3cform/converters.py Show resolved Hide resolved
plone/app/z3cform/converters.py Show resolved Hide resolved
@petschki petschki force-pushed the pat-contentbrowser-widget branch from 4118e58 to 62aff76 Compare September 16, 2024 12:35
@petschki petschki force-pushed the pat-contentbrowser-widget branch 2 times, most recently from 8d25b09 to 54ce09d Compare September 16, 2024 12:47
@petschki
Copy link
Member Author

@jenkins-plone-org please run jobs

@petschki petschki merged commit d38e1fa into master Sep 20, 2024
12 checks passed
@petschki petschki deleted the pat-contentbrowser-widget branch September 20, 2024 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

PLIP: Add contentbrowser as a replacement for relateditems
4 participants