Skip to content

Add a new behavior to link images on a risk#357

Open
ale-rt wants to merge 8 commits intomainfrom
ale/3447/images-behavior
Open

Add a new behavior to link images on a risk#357
ale-rt wants to merge 8 commits intomainfrom
ale/3447/images-behavior

Conversation

@ale-rt
Copy link
Member

@ale-rt ale-rt commented May 23, 2025

This is an MVP for the image selection widget in the risk edit form.

Requires the following checkouts:
We do not require them anymore because updated eggs they have been released in the meanwhile.

[sources]
# plone.app.content = git git@github.com:plone/plone.app.content.git branch=master
# plone.app.z3cform = git git@github.com:plone/plone.app.z3cform.git branch=master
# plone.z3cform = git git@github.com:plone/plone.z3cform.git branch=ale/29/fix

I would not merge this one yet because there is still quite a lot to do.

About image management

  • I could not find in proto how to add a new image, I had to add the images manually
    (calling /++add++Image)
  • There is no clear way to manage uploaded images (editing, deleting, ...)

About the relation management:

  • Only one relation can be picked at a time
  • Removing a relation is not yet implemented
  • When sorting the relations some injection happens (it should not)
  • When sorting the relations the images are (apparently) not sorted,
    but reloading the page shows them in the proper order

Minor UI issues:

  • The picker that opens in the panel has not the proper classes in the buttons
  • We do not have a proper macro for the tabbed panels
  • There is no page to display the image view
  • In the navigation tree we do not have an icon for the image

Rest API issues:

  • The related_images field is not serialized as null

@pilz
Copy link
Member

pilz commented May 27, 2025

I passed the missing image upload to D.

@pilz
Copy link
Member

pilz commented May 29, 2025

I tried that out, upgrade steps ran fine, works as described. Code style wise, I am no authority, but trust. Looks fine.

@ale-rt ale-rt force-pushed the ale/3447/images-behavior branch from fb0baf9 to b340a88 Compare August 22, 2025 10:16
Copy link
Contributor

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

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

I requested two small technical changes.
With those addressed, it works for me.

But there are still the points that Alessandro already mentions. Main ones being: there is no way in Proto to upload an image, and removing a related image is not yet implemented.

This also means that basically the entire Plone Site is now an image bank: the tool presents any Image it finds anywhere. Probably they should be somewhere central. But that probably means the image bank app itself should be implemented first. Or am I missing something?

So this is a good start, but it may only be mergeable once work in other areas is done.

@ale-rt ale-rt force-pushed the ale/3447/images-behavior branch from c26e9bc to b59fb24 Compare August 25, 2025 07:41
@ale-rt ale-rt requested a review from mauritsvanrees August 25, 2025 08:12
Copy link
Contributor

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

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

Thanks, the changes are good. I have left another suggestion, but it is that: a suggestion.

Technically I would approve, but there are still the open questions, mostly for Alex to consider.

@mauritsvanrees mauritsvanrees dismissed their stale review August 25, 2025 13:20

All required changes have been made.

@ale-rt ale-rt requested a review from pilz August 25, 2025 14:16
@ale-rt
Copy link
Member Author

ale-rt commented Aug 25, 2025

Thanks, the changes are good. I have left another suggestion, but it is that: a suggestion.

Technically I would approve, but there are still the open questions, mostly for Alex to consider.

CC @pilz, see also the issue description

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants