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

Some client side input validation #85 #119

Closed
wants to merge 23 commits into from
Closed

Conversation

Tuvyu
Copy link

@Tuvyu Tuvyu commented May 29, 2023

Hi, this is my first time doing a pull request, so, I apologize beforehand if I made any mistake. So, today, I noticed an open issue which was #85 and attempted to remedy it as much as possible.

I use Windows 10. And I have tested it out on Chrome, Firefox, Brave, and Opera Gx.
Except for Firefox, rest block the user from inputting any letters except the letter 'e'. And they all have been tested for negatives, floats, and seemed to do fine. There is a minimum of 5000 for both height and width.

Lastly, these are all on the client side and not server verified.

@netlify
Copy link

netlify bot commented May 29, 2023

Deploy Preview for competent-tesla-4b5f1e ready!

Name Link
🔨 Latest commit 46fd8e9
🔍 Latest deploy log https://app.netlify.com/sites/competent-tesla-4b5f1e/deploys/648e3ccda977cf000838e152
😎 Deploy Preview https://deploy-preview-119--competent-tesla-4b5f1e.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@unsettledgames
Copy link
Collaborator

Hi! Thanks a lot for the PR. A few things:

  • The same validation you did should be applied to all the number fields in the "Scale sprite" and "Resize canvas" windows. Should be easy enough to add
  • Please, move the validator function from Startup.js to Util.js, in that way it can be reused for other stuff in the app (see above). I'd also give it a better name if possible (e.g. validateNumberInput)
  • The resize canvas and scale sprite windows have different requirements. For example, a scale factor of 400% applied to a 1500x1500 canvas would result in a canvas bigger than 5000x5000. In cases like this, a custom range to provide to the validator function would probably be the way to go

For the rest it looks like it's working so thanks for your work!

@Tuvyu
Copy link
Author

Tuvyu commented May 29, 2023

Hi

Thanks for the detailed response. I appreciate it. I will do as you say!

@Tuvyu
Copy link
Author

Tuvyu commented May 30, 2023

Hello

I have made the changes. Placed numberValidator into Util.js, created four class properties in Filejs to keep track of incorrect inputs and prevent resizing accordingly. I have done some shallow testing and they seem to work.

Can you please take a look first? I will work on the custom range afterward.

Thank you!

@unsettledgames
Copy link
Collaborator

Thanks! The "Keep current ratio" button in the scale sprite window doesn't seem to be working anymore for some reason. The rest seems to be ok, maybe reset the window parameters when a bad one is inserted so that the user has a minimum amount of feedback :)

@Tuvyu
Copy link
Author

Tuvyu commented Jun 1, 2023

Yes, I was thinking of that and I even added a warning injection at first but I kept encountering some issues so I removed them while also considering I should not unnecessarily add things. But with your confirmation, I can work on it more flexibly now!

I will also check out the issue you mentioned.

@unsettledgames
Copy link
Collaborator

A simple warning div or maybe even an alert should be enough if you want to notify the user, that would be great if you have time, otherwise no problem and you can just reset the parameters

@Tuvyu
Copy link
Author

Tuvyu commented Jun 1, 2023

That's exactly what I had at first. I will retry building it out again as I understand the states more now. Last time, it kept memory leaking but it was also cause I did not have a proper validation structure.

@Tuvyu
Copy link
Author

Tuvyu commented Jun 2, 2023

Hi
The "Keep current ratio" does not seem to be working in the prod app too. It resets to 100% every time. I am unsure if you were talking about that.

@Tuvyu
Copy link
Author

Tuvyu commented Jun 2, 2023

Hello

I have made the changes and added a span feedback. I will look into the 'keep current ratio' issue moving forward but can you please look at this first?

Thank you

@Tuvyu
Copy link
Author

Tuvyu commented Jun 2, 2023

Hello

I think I fixed 'keep current ratio'. Can you confirm? Thanks.

@unsettledgames
Copy link
Collaborator

unsettledgames commented Jun 3, 2023

Almost there, if "keep ratio" is enabled, the same effect should be applied also when the "New sizes" fields are edited, not only the percentages

PS: great job for the invalid size messages

@Tuvyu
Copy link
Author

Tuvyu commented Jun 3, 2023

I figured so. Okay, will work on it in a bit!

@Tuvyu
Copy link
Author

Tuvyu commented Jun 3, 2023

Wait, I don't understand. I don't see the problem. Can you clarify?

@Tuvyu
Copy link
Author

Tuvyu commented Jun 6, 2023

Hello

I need more clarification cause it looks alright to me. Thanks.

@unsettledgames
Copy link
Collaborator

Hi, sorry for the delay, got a new job and I'm very busy lately.

Expected behaviour: the "keep ratio" button forces the ratio width / height to stay constant when the user edits one of the parameters. For example, width = 32, height = 32. User sets width to 64, height is automatically updated to 64 too and so are the percentages values (which are both set to 200%). If "keep ratio" isn't checked, then the updates don't happen.

At the moment this doesn't seem to happen, even though it works correctly when the user edits a percentage field. Hope this makes things clearer, sorry again for the delay :)

@Tuvyu
Copy link
Author

Tuvyu commented Jun 8, 2023

No worries. I also started a software developer internship a few days ago. I will take a look at this when I have time!

@Tuvyu
Copy link
Author

Tuvyu commented Jun 9, 2023

It should be fixed now. I had some class properties, sh and sw from some previous attempts that I forgot to remove which was causing the issue. It's resolved now. Please take a look!

@Tuvyu
Copy link
Author

Tuvyu commented Jun 11, 2023

bump!

@unsettledgames
Copy link
Collaborator

If you disable the "Keep ratio" button, edit the fields, re-enable the "Keep ratio" button and edit the fields, the button doesn't work. Example:

  • Set width percentage to 200%, all fields update correctly
  • Disable "Keep ratio"
  • Halve the width value
  • Enable the "Keep ratio" button
  • Double the height percentage

Correct behaviour: the other fields update as well
Observed behaviour: only the height percentage field updates

Please, try making a more thorough testing before submitting for review

@Tuvyu
Copy link
Author

Tuvyu commented Jun 17, 2023

I think I can see some consistent issues. But I feel like it will take me too long to figure this out. So, I do not plan to work on it anymore. If there is anything else, I can work on, I can. Otherwise, you can archive this issue and I can move on.

Thanks!

@unsettledgames
Copy link
Collaborator

I'll close this one then. Unfortunately, I don't see any other issues that could be easier to tackle than this one. Feel free to reopen it if you feel like it in the future, thanks anyways for the effort

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants