-
-
Notifications
You must be signed in to change notification settings - Fork 21.2k
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
Apply new input validation method for Create Plugin dialog #76778
Apply new input validation method for Create Plugin dialog #76778
Conversation
7ab802d
to
aa28f0d
Compare
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.
I left some style comments that need to be addressed, but otherwise looks good.
I think given how this validation convention is widely used now, it might be worth it to create a dedicated class to handle it. I have some idea, but it's for another PR.
aa28f0d
to
50af124
Compare
Is this an overall improvement though? We lose direct association between the error (or successful validation) and the field. |
Well, you could say the same about every dialog where we use this type of validation. |
Yes, you could. Which makes me skeptical about it in general. In some more complex cases it kind of makes sense. But here it's a simple form, and we already have nice validation, at least as far as immediate hints from icons are concerned. And we want to remove that for some text that you need to read and figure out and associate back with the form. |
#78744 will unify all these validations into a single class, which will allow us to easily modify it. Then we can e.g. associate specific errors with some fields. |
#78744 was merged, so this needs to be updated. It will simplify the code. |
50af124
to
0ef0332
Compare
The "subfolder valid" message still appears when you edit the plugin, even though the path field is not visible. I think the path message should not be displayed when editing. |
0ef0332
to
83c16ea
Compare
83c16ea
to
8671dc4
Compare
Thanks! And congrats for your first merged Godot contribution 🎉 |
Fix #76681