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

Field validation system 2 #2488

Merged
merged 15 commits into from
Nov 18, 2022

Conversation

Gazook89
Copy link
Collaborator

@Gazook89 Gazook89 commented Nov 5, 2022

This is the new take on input field validation, focused on the Properties Editor at this point. This PR replaces #2374, and I'll try to port over a good portion of the open discussion points to this comment, though I might need to do it another night. And I'll keep this top comment updated with to-do's.

  • ⚠️ Prevent write to props if field is invalid when autosave/save is triggered, keeping previous props value.
  • ⚠️ Add _.debounce() to thumbnail URL field to reduce requests to image host
  • Tie in the tags validating system
  • Set actual validation rules
    • Thumbnail URL:
    • Title:
      • (?) character limit
    • Description:
      • (?) character limit
  • Update CSS to turn invalid field to color: red or similar
  • Fix setState so it applies immediately (and triggers display of error message). Right now it is firing the "max 5 characters" rule only after the 7th character has been entered. I understand I need to set it up as an 'updater' function with the second argument in setState(), but I'm not really grasping how.
  • Create central Validators object
  • Let user navigate away at any time.

image

@Gazook89 Gazook89 mentioned this pull request Nov 5, 2022
3 tasks
@Gazook89 Gazook89 added the UI/UX User Interface, user experience label Nov 5, 2022
@Gazook89
Copy link
Collaborator Author

Gazook89 commented Nov 5, 2022

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".

I might need help with this part, @calculuschild @jeddai, if you have a chance. Right now it will save "Beelz", since on every stroke we are loading the value to props and there isn't anything "wrong" with it until we hit the 6th character:

if(Object.values(this.state.errs ?? {}).filter(Boolean).length === 0 || !this.state.errs[name]){
  e.target.setCustomValidity('');
  this.props.onChange({
	  ...this.props.metadata,
	  [name] : e.target.value
});

Does the .onChange() need to be .onBlur(), so it's only registering the entire text into props when you leave the field? edit: onBlur() definitely isn't the solution here

And @jeddai -- regarding the incorporation of the Tags validation, is that something you want to do on-top of this PR? I can also take a swing at it but will do it as a branch off of this branch.

fix an oversight in validations
@Gazook89 Gazook89 added the 🔍 R2 - Reviewed - Needs Help 🫱 PR is okayed but is stuck on an obstacle label Nov 6, 2022
@calculuschild
Copy link
Member

I might need help with this part, @calculuschild

I've given it a look over and it looks like it might involve a pretty heavy refactor of some pieces to get that behavior. I think I'm ok with the current behavior you have so as not to over-complicate this PR.

@jeddai
Copy link
Collaborator

jeddai commented Nov 12, 2022

Right now it will save "Beelz", since on every stroke we are loading the value to props and there isn't anything "wrong" with it until we hit the 6th character:

Does the .onChange() need to be .onBlur(), so it's only registering the entire text into props when you leave the field? edit: onBlur() definitely isn't the solution here

I think a good solution here would be to wrap a metadata editor change handler around the parent change handler. So if the metadata isn't valid, it would not apply those changes to the larger document, and it could be on its own 2.5s debounce. When the debounce ends we could have it override the normal debounce to save immediately and not have a total of 5s before the save.

And @jeddai -- regarding the incorporation of the Tags validation, is that something you want to do on-top of this PR? I can also take a swing at it but will do it as a branch off of this branch.

If you want to take a swing at it you're very welcome to, but I'm happy to branch off your changes and get it added as well. It's up to you!

@calculuschild
Copy link
Member

@jeddai I think that might be a good approach. Let's save it for a separate PR though since I think this PR does what it set out to do and we've accumulated a bunch of PRs now that are "mostly done" but are now stuck because I was asking for too many nitpick addons that they never get finished.

@jeddai
Copy link
Collaborator

jeddai commented Nov 13, 2022

That sounds reasonable to me!

@Gazook89
Copy link
Collaborator Author

Gazook89 commented Nov 13, 2022

I made the changes requested, except the tie-in with tags. Merge if you want for later addition of tags, or it can wait until it's all done. My kid just woke up so I won't come back to this until later.

@Gazook89 Gazook89 marked this pull request as ready for review November 14, 2022 01:11
@calculuschild
Copy link
Member

Thanks @Gazook89 ! Merging now! 🎉 💯 ⭐

@calculuschild calculuschild merged commit 3e44d0a into naturalcrit:master Nov 18, 2022
@Gazook89 Gazook89 deleted the Field-Validation-System-2 branch November 19, 2022 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔍 R2 - Reviewed - Needs Help 🫱 PR is okayed but is stuck on an obstacle UI/UX User Interface, user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add character limit to Thumbnail URL
4 participants