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

Required fields in form are still required even if field/group is hidden #9996

Closed
jardakotesovec opened this issue May 29, 2024 · 7 comments
Closed
Assignees
Labels
Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days. UI/UX Issues affecting the user interface/user experience
Milestone

Comments

@jardakotesovec
Copy link
Contributor

Describe the bug
Orcid form has use case, where first field enables Orcid and as result display rest of the form and some of the fields there are required. Problem is during validation, because the validation does not take in account if some fields are hidden.

Proposed solution
I think that improving form behaviour so it skips required fields if they are conditionally hidden would be expected behaviour.

What application are you using?
OJS, OMP or OPS version main

@jardakotesovec jardakotesovec self-assigned this May 29, 2024
@jardakotesovec jardakotesovec added the UI/UX Issues affecting the user interface/user experience label May 29, 2024
@jardakotesovec jardakotesovec added this to the 3.5.0 LTS milestone May 29, 2024
@jardakotesovec jardakotesovec added the Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days. label May 29, 2024
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue May 29, 2024
@jardakotesovec
Copy link
Contributor Author

@ewhanson This should resolve the workaround you are having in OrcidForm.vue. What you think?

ui-library: pkp/ui-library#357

@ewhanson
Copy link
Collaborator

Hey @jardakotesovec, thanks for this! I think this gets most of the way to solving my issues. It is possible to save a form with conditionally hidden required elements blank, but if there are previous errors, they are not dismissed when the fields are hidden (see video).

orcid_settings.mp4

jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 6, 2024
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 6, 2024
@jardakotesovec
Copy link
Contributor Author

jardakotesovec commented Jun 6, 2024

@ewhanson Ok.. additionally added removing error on field unmount, which can be think about as clean up for field which is not relevant anymore.

At this point I think this should work well.. can't think of where it could cause problems at this point.. but if you can think of something please let me know!

jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 6, 2024
@ewhanson
Copy link
Collaborator

ewhanson commented Jun 6, 2024

Thanks @jardakotesovec! This is most of the way there now, but I'm still running into an error. It seems that only one error is removed at a time when the components are unmounted. I have an example where there are 3 "required fields not completed" errors. And each time I hide/unmount the components, another one of the errors is removed.

orcid.mp4

jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 10, 2024
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 10, 2024
@jardakotesovec
Copy link
Contributor Author

@ewhanson Great catch, thanks! I did not test it with more than one field, so I did not notice it.

It went wrong, because how the state management is structured currently there. So every removing error created new errors object with that one error removed. But the errors object prop don't get propagated between these calls.. so all of them were modifying the original object and basically last change win.

Not fan of the solution I end up with.. hence the long comment in the code.. but it does work and I still think that this is how it should behave and it should remove the errors if the field gets removed.

@ewhanson
Copy link
Collaborator

Hey @jardakotesovec, that makes sense. I've done some testing with the latest changes and it's working as expected now. 👍

jardakotesovec added a commit to pkp/ui-library that referenced this issue Jun 13, 2024
#357)

* pkp/pkp-lib#9996 Remove error for input that gets hidden

* pkp/pkp-lib#9996 check only visible fields for required

* pkp/pkp-lib#9996 Handle correctly multiple fields removing error synchronously

* pkp/pkp-lib#9996 add jsdocs
@jardakotesovec
Copy link
Contributor Author

@ewhanson Thank you, merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days. UI/UX Issues affecting the user interface/user experience
Projects
None yet
Development

No branches or pull requests

2 participants