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

SafeguardInputs() cleaner #119

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

NoSloppy
Copy link
Contributor

@NoSloppy NoSloppy commented Jun 9, 2023

how about now?

if (!e.target.value) {
console.log("SafeguardInputs() run to avoid null value");
var changeEvent = new Event('change');
e.target.value = 0; e.target.dispatchEvent(changeEvent);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely better, but I'm wondering if it would better to just mark it in red and let the user fix it, or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played with it a good bunch last night. It's smooth, non-intrusive, and assumes if you deleted it that you wanted zero.
Red flag would be more of a distraction?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the case that I'm worried about is if you managed to enter something like 22123r5, but you meant to enter 221235, and if it gets replaced with zero, then you don't get a chance to fix it, you just have to re-enter it. However, I'm not certain if this can even happen if the fields have been specified to only take numbers as input.
Also, I'm not sure if there are any values would be falsy that would be worth keeping, or if the if statement only catches the empty string.

I think the code here is probably fine, it's not really obvious that it's fine though, which is why I worry I guess.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on the browser, you can sometimes put text into a number input. That does return falsy

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe we should change line 1260 from if (!e.target.value) to if (e.target.value == "")

Copy link
Contributor Author

@NoSloppy NoSloppy Jun 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. I changed the line as above to (e.target.value == ""). I also added oninput to the Alt value so it behaves like Variant, where typing updates the value in real time.
Generally speaking, it seems the same, and works fine IMO.

For a longer, more detailed explanation of use, I will type below, but it's probably much easier just to experience it:
https://nosloppy.github.io/ProffieOS-StyleEditor-1/style_editor_BC_V6.html

Several behavioral cases exist so I'll explain tests one by one.
The 2 factors are different browsers, and the difference between how Variant and Alt operate.
First, Chrome browser:

  • non numerical input is ignored, except for "e". If the value is deleted completely, or if "e" is typed, the field clears out. At this point, typing a number instantly updates the value to the OS, or leaving the field blank will enter a zero onfocusout.
    Variant - Same behavior, however you can see that when the field clears out, the slider moves to halfway. Now doing onfocusout will set the value to zero, but then using the + or - button will start from 16384 instead of zero. So that needs a tweak to update the VARIANT_SLIDER correctly, but I'm getting burnt out on this.

Safari and Firefox:
Same behavior, except that non-numerical input clears the field.

Chrome Mobile (didn't test other browsers) but the popup keyboard only offers numbers.

*edit - the ArgString inputs don't react in real time. Non-numerical is shown, but onfocusout is treated the same, resulting in a zero value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had coffee.
Fixed slider sync.

@NoSloppy NoSloppy changed the title Safeguard inputs() cleaner SafeguardInputs() cleaner Jun 13, 2023
@NoSloppy
Copy link
Contributor Author

This is getting dusty. God forbid I need to remember what we did, but I can see it was left in a good place.
Anything else?

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.

3 participants