Skip to content

Fix Image add Content tab crash and the form close control. - #164

Open
aryan7081 wants to merge 7 commits into
plone:mainfrom
aryan7081:fix-image-content-tab-widget
Open

aryan7081 wants to merge 7 commits into
plone:mainfrom
aryan7081:fix-image-content-tab-widget

Conversation

@aryan7081

Copy link
Copy Markdown
Contributor

Fixes #163

This issue happened because the Image add Content tab spread the full JSON Schema onto React Aria widgets, so object fields crashed the route. The image object browser then fetched /@objectBrowserWidget/@@add, which is a CMS route rather than a content path, so React Router had no result for that resource. Close did nothing on add because it linked to content['@id'], which a new item does not have yet. Save then failed because the Image field stored a URL string while createContent expects a NamedBlobImage object (data, encoding, content-type, filename).

This fix registers ImageWidget for REST API Image and file fields, forwards only a small set of widget props, maps /@@add and /@@edit to a real content path before loading the object browser, and points Close at that path (or the item @id on edit). Uploads write a NamedBlobImage payload; picking an existing image copies that file into the same shape so Save can succeed. Image scale flattening also skips missing scales/download so blob images do not take down the view.

Caution

The Volto Team has suspended its review of new pull requests from first-time contributors until the release of Plone 7, which is preliminarily scheduled for the second quarter of 2026.
Read details.



If your pull request closes an open issue, include the exact text below, immediately followed by the issue number. When your pull request gets merged, then that issue will close automatically.

Closes #

@silviubogan silviubogan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

HTH. The bugs in the UX look solved on my side. Congrats!

const fetcher = useFetcher();
const location = useLocation();
const cancelHref =
content['@id'] || getContentPathFromCmsUrl(location.pathname) || '/';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

|| '/' is useless because getContentPathFromCmsUrl cannot return falsy value because location is a Location object which cannot have a falsy pathname because, as written in the docs: https://api.reactrouter.com/v8/interfaces/react-router.Location.html#pathname, pathname always starts with an / and so is never an empty string (which would be falsy).

typeof extraFieldProps.description === 'string'
? extraFieldProps.description
: undefined,
placeholder: fieldProps.placeholder || 'Type something...',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe that this string should be translateable.

const cli = context.get(ploneClientContext);

const path = `/${params['*'] || ''}`;
const path = getContentPathFromCmsUrl(`/${params['*'] || ''}`) || '/';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think here we have the same issue: getContentPathFromCmsUrl cannot return falsy values.

return (
<ObjectBrowserProvider config={{ ...rest, initialPath: content?.['@id'] }}>
<ObjectBrowserProvider
config={{ ...rest, initialPath: loaderData?.content?.['@id'] || '/' }}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it a good idea to put || getContentPathFromCmsUrl(location.pathname) before the ||, where location is retrieved with useLocation from React Router?

import type { RootLoader } from '@plone/aurora/app/root';

function removeObjectIdFromURL(basePath: string, scale: string) {
if (typeof scale !== 'string') return scale;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

According to the argument type scale: string in the first line above, this condition is never true.

download: removeObjectIdFromURL(basePath, image.download),
};

if (!imageInfo.scales || typeof imageInfo.scales !== 'object') {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typeof imageInfo.scales !== 'object' is true when !imageInfo.scales, so we can keep only the second condition, imageInfo.scales !== 'object', if we are sure that imageInfo.scales is never null which has type object.

contentId: string,
filename: string,
): Promise<NamedBlobImage | null> {
const path = contentId.startsWith('/') ? contentId : `/${contentId}`;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it probable that contentId can either start or not with / in this place?

This branch has not been deployed

No deployments
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.

Add Image Content tab crashes because the image field is rendered as a TextField

2 participants