Metadata field validation

[Gazook89]

This is to fix #2356 by providing a validation error for too many characters in the thumbnail URL field. It is setup to provide a regex-based method of setting field requirements that will generate an error bar above the editor. Hopefully this means it can be used on any text fields with any regex.

Upon review by others, I'm bracing myself for someone to say "oh, why didn't you just....." with a much simpler solution.

**NOTE: with this draft, the PR has silly rules for easy testing:

1. Description field must have no more than 5 characters
2. URL field must have no more than 5 characters
3. URL field must start with "W".

Some issues:

• [ ] This prevents write to the props.metadata if errors exist (even for valid fields)....this doesn't bother me much, let me know what you folks think.
• [ ] I have a max character count set to 5, using regex /^.{0,5}$/ but it seems like it's allowing 6 characters to write to props. I'm sure this is some state or React thing I don't understand?
• [ ] Should there be a way of minimizing these errors so they are not in the way? I don't think dismissing them entirely is good, but a button to reduce in size, or after a short period of time?

[Gazook89]

Oh, and I won't be bummed if there is a totally different direction to bring this-- as with all my PR's, it's a learning thing first and foremost.

[calculuschild]

@jeddai What do you think about this one? Would this be possible to integrate with your Tags validation to help with #2357 ?

[Gazook89]

I can comment the code better too if needed (or I hope I can) especially if others need to grab from it. I’m not sure how it would integrate with the tag component but also I haven’t looked at that at all.

[jeddai]

Form validation is an area I have a lot of opinions on so I'm going to apologize in advance...

[jeddai]

@jeddai What do you think about this one? Would this be possible to integrate with your Tags validation to help with #2357 ?

When it comes to integrating with the tags validation, that one was a tad different since it was designed to be extensible from that component, and this would be a step above that effectively. I may also be misunderstanding what you're suggesting.

[jeddai]

Something else I'll generally comment on is I definitely would argue for having the errors be shown underneath the field(s) that are causing the error(s) as appropriate.

Some examples in existing libraries:

• Bootstrap
• Angular Material

[calculuschild]

When it comes to integrating with the tags validation, that one was a tad different since it was designed to be extensible from that component, and this would be a step above that effectively. I may also be misunderstanding what you're suggesting.

The current tags validation prevents updates and turns the tag red, but it doesn't display the reason why it's invalid. I'm mostly thinking if we are going to add a validation and error output to the metadata panel, we might want to have the tags share the same system.

[jeddai]

Ah yes I'd definitely agree.

[calculuschild]

This might need to be handled between @Gazook89 and @jeddai due to the overlap between components. Maybe we need to build up a checklist of what needs to be completed to finish this out.

Just based on the above comments:

• [ ] Move error to display below the selected element (is there even room?)
• [ ] Create a central "validators" object
• [ ] Get Tags to use the same core error system

[Gazook89]

Move error to display below the selected element (is there even room?)

I had the same thought about spacing when I was considering where to place the error, which is why i went with an overlay. I understand the spacing needed for an error message appearing underneath a field when filling out a field once then submitting to never seeing it again, but with a set of fields like this I don't like that much spacing. But perhaps it's not a big deal to increase that row spacing so that errors show below (with large enough font size).

If continuing with an overlay, there should be a way to dismiss each error message individually, though. I don't think it's likely, but if there are enough error messages that you can't see or click into the text box, that would be bad.

[calculuschild]

@Gazook89 Let's see if we can finish this PR out. Perhaps let's start with the central validator object as @jeddai suggested https://github.com/naturalcrit/homebrewery/pull/2374#discussion_r973665731

[ericscheid]

Also, is there a debounce on the fields?

That is, to avoid hammering someone's image server for a url that is increasing one character at a time?

https://homebrewery.naturalcrit.com/t https://homebrewery.naturalcrit.com/th https://homebrewery.naturalcrit.com/thu https://homebrewery.naturalcrit.com/thum https://homebrewery.naturalcrit.com/thumb https://homebrewery.naturalcrit.com/thumbn https://homebrewery.naturalcrit.com/thumbna https://homebrewery.naturalcrit.com/thumbnai https://homebrewery.naturalcrit.com/thumbnail https://homebrewery.naturalcrit.com/thumbnail. https://homebrewery.naturalcrit.com/thumbnail.p https://homebrewery.naturalcrit.com/thumbnail.pn https://homebrewery.naturalcrit.com/thumbnail.png

[G-Ambatte]

I don't believe that there currently is, but it would be a sensible thing to do. Lodash's debounce() should be perfect for exactly this function - https://lodash.com/docs/4.17.15#debounce

[Gazook89]

Gitter Conversation:

calculuschild I think under field. It takes up some space, but I think it will be more clear to the user. We can potentially "animate" the appearance of the error to push the other elements down so they don't take up space until an error occurs. A popup could work as well as long as it pops up at the site of the error instead of just one universal banner. My only concern is it might cover up the other fields which could be obnoxious.

Gazook89 if doing errors under the associated field i will likely increase the spacing between rows just a bit. I think its best avoid animating those things if possible, and instead try our best to make the err Message as succinct as possible. If we end up going with larger messages or throwing multiple errors on a single field, maybe animating a shift down is the only way to go

calculuschild Yeah. I'm hoping most errors will be just a few words.

Gazook89 or, pop a red X button on the field (either right on the input or on the label to the left) with a tooltip on hover?

[calculuschild]

or, pop a red X button on the field (either right on the input or on the label to the left) with a tooltip on hover?

Let's just show the error directly. Eliminate any extra steps for the user to see what is wrong.

[Gazook89]

Let's just show the error directly.

Okay

New question 1)

should we prevent users from navigating away from the properties editor if there are invalid values?

New question 2)

If there is an invalid input, should we invalidate the entire value and set the prop to null or whatever it's typical default is? Or do just take whatever the last valid value was and save it to props?

For example, let's say the rule for thumbnail is "must be less than 5 characters". If they type "beezlebub", do we chuck out their entry and just set the prop.metadata.thumbnail = null?

Or should the entirety of "beezlebub" be allowed to be entered, but only "beez" be saved to props (and then if they navigate out of properties editor and back into it, the field now says "beez")?

[calculuschild]

1. Let the user navigate away at any time.

2. we should only be saving properties when the brew itself saves. As in, say the value is "Satan" from the last time they edited it. Valid because it's <= 5 characters. They then go in and start typing, until it becomes "Beelzebub", let go of the keyboard, and then 2500 ms pass, triggering auto save. Since the field is currently invalid, we just don't update that field. I.e., it should stay as "Satan" if they refresh the page now, rather than a partial entry of "Beelz".

This means if the user navigates away from the properties menu, then goes back, it should refresh with the last saved values, not the invalid value they left behind.

That's my opinion anyway.

[Gazook89]

👍 on both answers.

One more:

right now the setup is "Allow multiple rules per field". This is beneficial to me because each rule is easier to read and write....less complicated regex / expressions. Also, it allows more specific error messaging for whatever rule is being broken.

If a field has multiple rules broken ("doesn't start with W and is also longer than 5 characters"), should both error messages be visible? Will this crowd the small space available? Or just display one error at a time, and once that is fixed show the next error? Perhaps this could look something like this: URL must start with "W". [+1] changing the +1 to whatever number of errors.

EDIT::

I can manually set the html input to actually be "invalid" (such that the css pseudo-selector :invalid picks it up) and with a custom message. And that message can be displayed using the browsers built-in validation error, which in Chrome looks like this:

It fades after a short time, so I would also set the invalid input to red text or red border. But this gets around the "limited space" issue and uses the browsers own tools. Thoughts?

[calculuschild]

Hmm.... Perhaps just one error at a time... @jeddai ?

[jeddai]

I like that last solution you proposed @Gazook89, of using the built-in invalidity piece to display multiple errors at once if there is more than one.

I might suggest not numbering them, but bulleting them should be good!

[Gazook89]

I'm going to close this after opening a new effort on PR #2488 which is current with the upstream/main. I'll bring over open discussion points.