-
-
Notifications
You must be signed in to change notification settings - Fork 742
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
Volto form block fixes #6370
base: main
Are you sure you want to change the base?
Volto form block fixes #6370
Conversation
✅ Deploy Preview for plone-components canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add release notes and fix the lint errors and failing unit tests.
This PR should also render its Storybook. We recently merged a PR that should fix the pull request preview build on Netlify: The Netlify build currently fails to recognize changes in the most recent commit: |
6d9bc8a
to
be53373
Compare
988b47e
to
dd5a050
Compare
This caused the object_browser widget loosing the mode and return props which got the default "multiple", "multiple" values even when "image", "single" was desired. As a consequence this broke the preview images.
dd5a050
to
dc2c6a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed only the .pot
and news, both of which look good, with a couple of MyST markup enhancements and a question.
Will this PR need developer documentation? For example:
packages/volto/src/components/manage/Widgets/CheckboxGroupWidget.jsx
Outdated
Show resolved
Hide resolved
The issues are fixed. The page for those widgets is about declaring widgets in the backend not the frontend so doesn't make sense to add anything on that page. |
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this has a lot of useful improvements, but still some rough edges and missing docs.
/** | ||
* On updates caused by props change | ||
* if errors from Backend come, these will be shown to their corresponding Fields | ||
* also the first Tab to have any errors will be selected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code doesn't appear to have anything to do with form errors?
@@ -346,6 +347,7 @@ const blocksConfig = { | |||
view: ViewVideoBlock, | |||
edit: EditVideoBlock, | |||
schema: BlockSettingsSchema, | |||
blockSchema: VideoBlockSchema, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for? I don't see anything about the video block in the changelog.
!schema.properties[requiredField].readonly && | ||
isEmpty) || | ||
(schema.properties[requiredField]?.type === 'boolean' && | ||
formData[requiredField] !== true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is a bit dangerous. It's a breaking change, and for schemas coming from the backend, required
on a Boolean means it has to have a value of True or False rather than None, not that it has to be True.
import renderer from 'react-test-renderer'; | ||
import HiddenWidget from './HiddenWidget'; | ||
|
||
describe('TextWidget', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
describe('TextWidget', () => { | |
describe('HiddenWidget', () => { |
/* webpackChunkName: "Widgets" */ '@plone/volto/components/manage/Widgets/TimeWidget' | ||
), | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably the new widgets need to be mentioned in docs somewhere?
const blockConfig = | ||
blocksConfig[properties[getBlocksFieldname(properties)][block]['@type']]; | ||
|
||
if (e.key === 'Enter' && !disableEnter && !blockConfig.disableEnter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disableEnter in blockConfig is not documented
@@ -982,6 +1002,8 @@ class Form extends Component { | |||
)} | |||
{map(schema.fieldsets[0].fields, (field) => ( | |||
<Field | |||
widgets={this.props.widgets} | |||
component={this.props.component} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you pass component
to the Field here and on line 949? I don't see where it is used?
Add optional widgets `config` to form component. @robgietema | ||
Add option to override the form component and the buttons in the form component. @robgietema | ||
Add option to override cancel label in form. @robgietema | ||
Add option to choose bewteen icon and text buttons in the form component. @robgietema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add option to choose bewteen icon and text buttons in the form component. @robgietema | |
Add option to choose between icon and text buttons in the form component. @robgietema |
@@ -272,22 +283,19 @@ class Form extends Component { | |||
selected: null, | |||
}); | |||
} | |||
if (requestError) { | |||
|
|||
if (requestError && prevProps.requestError !== requestError) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a bugfix which isn't mentioned in the changelog.
title={this.props.schema.properties[err].title || err} | ||
content={errors[err].join(', ')} | ||
/>, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing in changelog?
type: 'string', | ||
title: intl.formatMessage(messages.queryParameterName), | ||
description: intl.formatMessage(messages.queryParameterNameDescription), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried adding a queryParameterName to a field in a content type schema, but it doesn't do anything.
type: 'string', | ||
title: intl.formatMessage(messages.parentFieldSet), | ||
choices: makeFieldsetList(fieldsets), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be shown even if there is only one fieldset available, which is distracting.
placeholder: { | ||
type: 'string', | ||
title: intl.formatMessage(messages.placeholder), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried adding a placeholder to a text field in a content type schema, but it didn't show up when I edited the content type.
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 #