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

feat: remove deleted image from content #7310

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hip3r
Copy link
Contributor

@hip3r hip3r commented Oct 22, 2024

Summary

Added configuration option for removing deleted image from content. Instead of saving empty string.
closes #7120
closes #7186

I wasn't sure that this should be working by default, that's why I extended cms configuration with new option remove_empty_image_field. conected PR for docs will follow.

Implementation is overlapping with logic for #3314 , which inserts default values for all removed entries. The same reason prevented me from saving image value as null or undefined instead of empty string.

Test plan

BEFORE:
image

AFTER:
image

Checklist

Please add a x inside each checkbox:

A picture of a cute animal (not mandatory but encouraged)
photo_2024-10-22_10-19-18

Copy link
Member

@demshy demshy left a comment

Choose a reason for hiding this comment

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

Instead of prop drilling the config, it might be more efficient to read the config once before calling serializeValues and add this option to the fields that are passed into the function. The state should be accessible on that point, I think. This approach would also maintain the possibility for adding this feature to specific fields in the future (or now?). What do you think?

We also have a chance to solve two problems in one go here and extend this to other field types (@martinjagodic I think this might interest you).

@@ -3,6 +3,8 @@ import { Map, List } from 'immutable';

import { getWidgetValueSerializer } from './registry';

const _pathsToRemove = new Set();
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we could find a way to make this non-global?

@martinjagodic
Copy link
Member

martinjagodic commented Nov 13, 2024

We also have a chance to solve two problems in one go here and extend this to other field types

We don't have to do this in this PR, but I would anticipate such a feature in config. So instead of remove_empty_image_field we could have remove_empty_fields, which would accept an array of widgets?

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