-
-
Notifications
You must be signed in to change notification settings - Fork 693
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
Displays validation error messages on control panel forms #5950
Conversation
✅ Deploy Preview for volto canceled.
|
✅ 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.
Could you resolve the merge conflict?
d465ffc
to
5cca767
Compare
@ichim-david I ran into the error in the "As editor, I can unlock a locked page" test again. I remember you found a solution to the problem: Do you intend to make this fixe? |
@davisagli done. |
@wesleybl yup I do ... time to dust up the old pull request I didn't add, April was not a good month for Volto work unlike March :) |
@wesleybl only left it there for 1 month .. if you would like to test it locally the locking test here is the pull request where I made the changes necessary to avoid errors after running it 20 times in a row as I've given you the comment on how todo it |
@davisagli @sneridagh can you take a look please? |
@wesleybl what's left of this one? |
63a7238
to
0e627af
Compare
@sneridagh I think it's ready to merge. There was a conflict but I resolved it. |
nextProps.updateRequest.error?.response?.text || | ||
''; | ||
|
||
const error = |
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.
@wesleybl is p.restapi returning html as error? Where? It's wrong, and should be fixed, at least file an issue.
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.
@sneridagh In fact, I only saw restapi return html when we used
debug-exceptions = on
in buildout. To see: plone/plone.restapi#1734
But perhaps this is an extreme case that is not even worth considering.
I just followed the way it is done when validating content forms:
volto/packages/volto/src/components/manage/Add/Add.jsx
Lines 178 to 180 in 6f18663
const error = | |
new DOMParser().parseFromString(message, 'text/html')?.all[0] | |
?.textContent || message; |
I also don't quite understand why this code is necessary. In my opinion it can be removed.
Perhaps @avoinea can explain why this is necessary, since he was the one who implemented the original code in:
0782a50#diff-0356a7af508f860d87802dfa2b06d502aaed9cd60b130e57740806bd956be87e
packages/volto/src/components/manage/Controlpanels/Controlpanel.test.jsx
Show resolved
Hide resolved
packages/volto/src/components/manage/Controlpanels/__snapshots__/Controlpanel.test.jsx.snap
Show resolved
Hide resolved
0e627af
to
f81c3df
Compare
@sneridagh I answered your questions. Can you take another look please? |
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.
@wesleybl Thanks for your work on this -- and I'm sorry it took me so long to review it.
Fixes: #5274
Now the error is displayed:
To works properly, it needs: plone/plone.restapi#1771
But it can be merged separately without any problems.