search

freeCodeCamp / league-for-good

An open source sports league management tool
BSD 3-Clause "New" or "Revised" License
163 stars 106 forks source link

fix assign player form validation #89

Closed IsaacRaymond closed 6 years ago

IsaacRaymond commented 6 years ago

meant to address issue #84

IsaacRaymond commented 6 years ago

Sorry, I thought I was being fancy by asking for one correction at a time. If a user has multiple fields empty/in error, we want them to all display at once, then?

IsaacRaymond commented 6 years ago

Gah... I know what you mean now. Sorry! I'll fix it

IsaacRaymond commented 6 years ago

I believe I've updated this PR correctly. As I'm brand new to working with others in Github, let me know if I've done something wrong!

makkoli commented 6 years ago

@IRGames Hey, you did your PR fine. I would actually check the numbers in a range using logical operators instead of regex. That is, cast it to a number and do jerseyNum >= 0 && jerseyNum <=99.

IsaacRaymond commented 6 years ago

@makkoli Thanks for the reply and feedback!

I actually used addPlayerFormValidation.js as a reference for this problem. Using regex made the code cleaner than anything else I could come up with. val.team.jerseyNum needs to be defined before I can transform it into an integer and check it.

I've updated updated the PR using logical operators, but feedback is, as always, welcome!

IsaacRaymond commented 6 years ago

Also, I noticed another issue here:

If a user starts typing a team name or a player, but deletes it, the user can submit empty fields. This doesn't happen in the validation of the "Add a player to the league" form. I'll look into resolving this as well!

makkoli commented 6 years ago

@IRGames You don't actually have to check if the jersey number is a number type. I was saying you should do a conversion to the number type using Number(val.team.jerseyNum), since I believe the input box returns it as a string(though I may be mistaken). If that is the case, then this would always throw an error since it's a string.

I'm not sure about the second issue. May be a redux-form thing. I've been wanting to remove redux from the app and move to simpler state management inside the components.

IsaacRaymond commented 6 years ago

@makkoli I included the check to avoid the console warnings about null values not being numbers - might have overkilled there. Removing that check causes a warning to the effect of "this is not a number".

Should I look into changing anything further with this pull request? Just let me know! I'm happy to learn what's conventional in programming and make the necessary changes.