search

winterstein / sogive-app

The SoGive user app - See your impact
Other
2 stars 5 forks source link

Fix Impact Calculator bug #246

Closed anitawoodruff closed 3 years ago

anitawoodruff commented 3 years ago

Previously, after the user manually updated the impact calculator, the raw value they typed would always be displayed, regardless of pressing + / - buttons. You can observe this behaviour at test.sogive.org.

Now, the impact calculator behaves correctly - it updates based on both user input and increment/decrement buttons.

Unclear to me exactly how this change fixes it - I think it's something to do with this comment in PropControl.jsx: 

    // What is rawValue?
    // It is the value as typed by the user. This allows the user to move between invalid values, by keeping a copy of their raw input.
    // NB: Most PropControl types ignore rawValue. Those that use it should display rawValue.
    // Warning: rawValue === undefined/null means "use storeValue". BUT rawValue === "" means "show a blank"
winterstein commented 3 years ago

Hm: So rawValue trumps everything for display, hence the bug. But this fix might create a different bug -- rawValue allows for invalid input like 0. as the start of typing 0.50. Having (+) and (-) clear the money.rawValue might be an alternative fix.

anitawoodruff commented 3 years ago

Hm: So rawValue trumps everything for display, hence the bug. But this fix might create a different bug -- rawValue allows for invalid input like 0. as the start of typing 0.50. Having (+) and (-) clear the money.rawValue might be an alternative fix.

I tested and it does not accept an input at 0 at the start of the field - if you type one it's ignored. Same if you type anything aside from digits anywhere in the box (decimal points don't get inputted either). Couldn't get the calculation to break whatever I typed with this fix.

We did initially try having (+) and (-) clear the money.rawValue, but couldn't figure out how to do that, despite spending an hour on this with Aishah. Everything else we tried failed.

Given this does not seem to introduce any other bugs, can you reconsider merging it?