-
-
Notifications
You must be signed in to change notification settings - Fork 740
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
Improve the usability of the ObjectBrowser when inputting a manual value, checking it on blur, and adding a local validator #6576
Conversation
…lue, checking it on blur, and adding a local validator.
✅ Deploy Preview for plone-components canceled.
|
return ( | ||
<FormFieldWrapper | ||
{...this.props} | ||
{...props} |
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.
It's confusing to have a thing called props
which is more than just the props. Can't you just pass error
separately here?
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.
@davisagli check now it's better.
Thanks to the errors I've also found out that the OBW errors is messed up because how we use it as a HOC in some places :/ The errors shape switches back and forth from {}
to []
... We didn't realise because we never validated what's inside. Now we are dealing with a little validation inside, and since we do not handle outside validation, let's simplify it and do not take that into consideration. We can do it if we enable external validation in this widget at some point.
@plone/volto-team please a look on this one? |
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.
News is OK. Needs technical review.
if (error && url !== '') { | ||
this.setState({ errors: [error] }); | ||
} else { | ||
this.setState({ errors: [] }); |
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 isn't this redundant due to the initial empty value at line 105?
https://github.com/plone/volto/pull/6576/files#diff-526a68594a32e6dd3a09f1243f7780736303aae2414bb98f8670cbdbc5f71703R105
Technically speaking react should know not to trigger a state chance if it's the same value but I
still think this is unnecessary
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.
@ichim-david this is required to reset the state in case that the error is gone.
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.
We could reset the state at the top of the method too.
Co-authored-by: Steve Piercy <web@stevepiercy.com>
* main: (741 commits) Improve the usability of the ObjectBrowser when inputting a manual value, checking it on blur, and adding a local validator (#6576) fixing Markdown heading (#6588) fix site logo issue (#6591) Route registry (#6600) Release @plone/client 1.0.0-alpha.21 Export the getContent bare fetcher (#6594) fix: incorrect copied state useClipboard (#6585) [RR7] Update to latest RR7 and conventions, fix index page (#6589) Release 18.6.0 Release @plone/slate 18.1.0 Revert "added swedish translation" (#6578) Slate Italian translations (#6563) Release 18.5.0 Fix robots.txt in devmode (#6571) added swedish translation (#6557) Depth search issue (#6558) Block examples documentation (#6560) Fixed folder contents issues with persistent selection (#6554) Fix redirects to MDN responsive images (#6552) Bugfix remove query string inclusion in body class generation logic (#6547) ...
Instead of requiring to press enter to validate the value, it does it on blur too.
Also, moved to use the URL validator from the main Validators.