viritin / flow-viritin

Viritin inspired project for Vaadin Flow
Other
43 stars 16 forks source link

[discussion] Argument for keeping the validation violation handling open in `FormBinder` #64

Closed christoph-frick closed 3 months ago

christoph-frick commented 4 months ago

Currently, the "map based" handling of constraint violations (setRawConstraintViolations) is marked as deprecated. I'd like to argue the point here to make something generic the default and actually make the implementation for jakarta.validation the special case.

While the map based approach might indeed be just "too generic", it allows using any "validation library". And while jakarta.validation for sure is a staple for the JVM and many will use it (by default), it's not the only one.

If your project decided on using a different approach -- be it a different library or rolling your own -- it might rule out using the niceties of FormBinder just because it is opinionated (which I don't mind, I just need to know).

Sorry for making this an issue to start a discussion.

mstahv commented 3 months ago

I heard similar comments from one of our architects that he tends to avoid the BeanValidation API. I'd argue that in his case it was just because of past experiences, but I'm also now towards not giving any strong suggestions here. Will remove that.

In the same discussions, a need to highlight "complaints" with different level, error would be red, warnings would be orange and so on. With BV that migh be bound to "groups", but didn't check if the API would there as well need some argument for "level" of the violations. Any similar needs on our side? Probably can be added in backwards compatible manner though.

mstahv commented 3 months ago

2.8.19 in syncing to central.

christoph-frick commented 3 months ago

In the same discussions, a need to highlight "complaints" with different level, error would be red, warnings would be orange and so on. With BV that migh be bound to "groups", but didn't check if the API would there as well need some argument for "level" of the violations. Any similar needs on our side? Probably can be added in backwards compatible manner though.

The levels for sure have use-cases; even a "success" sometimes makes sense to show ("all is fine, here is your green checkbox"). One problem though is, that HasValidation only allows for a string as error message, and that this is probably is the "best" way to show an error with a field (e.g. tested, UX, ...). Yet I assume, that marking the field invalid and show a component as validation result somewhere else is supposed to work?

So musing about it, the generic approach could be to have an interface to run the validation checks and return a generic validation result, that holds:

Then there is the renderer; it just accepts the results and applies it to the UI. The generic default renderer implementation would just do what FormBinder currently does: if the anchor is a known field, put the invalid-flag and message on it, otherwise put the message in the "class level" block. Or just pass your own, which creates fancy components left and right.