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

Fix bug in discount field #614

Merged
merged 1 commit into from
May 26, 2023
Merged

Conversation

JooVLC
Copy link
Contributor

@JooVLC JooVLC commented May 23, 2023

Fixing the type handlePremiumChange function to handle the discount to the right type (React.ChangeEventHandler<HTMLInputElement | HTMLTextAreaElement>)

Using the correct type of handlePremiumChange function, then deconstructing the "value" var from the event.target

Using Number(value) in declaration of "newPremium", instead of using multiplication with number and string

To not affect another methods using "premium" var, I used isNaN func to make a check and set the initial value of "premium" var to zero (if is NaN) or newPremium (when valid number is entered)

The fix in the bug is the line "premium: isNaN(newPremium) || value === '' ? '' : premium," in setMaker. That makes the "premium" var to be set to zero when the input is empty or not a number, allowing the user to input the "-" (minus sign) and not entering 0 as before.

What does this PR do?

Fixes #532

This PR fix the bug in discount field changing the function handlePremiumChange in "frontend/src/components/MakerForm/MakerForm.tsx".
This changes "frontend/static/locales/collect_phrases.py" too, because the pre-commit was running in loop, changing the locales's json. I added a comparison in the python script to not update the json if he no has changes.

Checklist before merging

  • Install pre-commit and initialize it: pip install pre-commit, then pre-commit install. Pre-commit installs git hooks that automatically check the codebase. If pre-commit fails when you commit your changes, please fix the problems it points out.

Fixing the type handlePremiumChange function to handle the discount to the right type (React.ChangeEventHandler<HTMLInputElement | HTMLTextAreaElement>)

Using the correct type of handlePremiumChange function, then deconstructing the "value" var from the event.target

Using Number(value) in declaration of "newPremium", instead of using multiplication with number and string

To not affect another methods using "premium" var, I used isNaN func to make a check and set the initial value of "premium" var to zero (if is NaN) or newPremium (when valid number is entered)

The fix in the bug is the line "premium: isNaN(newPremium) || value === '' ? '' : premium," in setMaker. That makes the "premium" var to be set to zero when the input is empty or not a number, allowing the user to input the "-" (minus sign) and not entering 0 as before.
@JooVLC
Copy link
Contributor Author

JooVLC commented May 23, 2023

@Reckless-Satoshi, I tested the changes and it appears to be working great, but I will run more tests soon. If anyone wants to test too be welcolme.

After more tests I will put the PR for review.

Another detail is I had to change "frontend/static/locales/collect_phrases.py" because the pre-commit was running in a loop.
If you can verify this change too. I can put it in another PR, if needed.

with open(locale, "w", encoding="utf-8") as f:
json.dump(new_phrases, f, ensure_ascii=False)
# don't change the file if there aren't new keys (order matters)
if new_phrases != old_phrases:
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for this fix! Indeed, there was a bit of a formatting-recursion-hell :)

updateAmountLimits(limits.list, fav.currency, premium);
setMaker({
...maker,
premium: isNaN(newPremium) || value === '' ? '' : premium,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This fix should work well!

@Reckless-Satoshi
Copy link
Collaborator

If you agree, I believe this PR is ready to be marked as ready for review (and subsequently merged 🚀 )

Please, accept a tip of 40K Sats from the devfund. Submit an invoice with a long expiration time from a proxy node id :)

@JooVLC
Copy link
Contributor Author

JooVLC commented May 25, 2023

If you agree, I believe this PR is ready to be marked as ready for review (and subsequently merged 🚀 )

Please, accept a tip of 40K Sats from the devfund. Submit an invoice with a long expiration time from a proxy node id :)

I agree, I will change the PR.

@JooVLC JooVLC marked this pull request as ready for review May 25, 2023 13:27
@Reckless-Satoshi
Copy link
Collaborator

If you agree, I believe this PR is ready to be marked as ready for review (and subsequently merged rocket )
Please, accept a tip of 40K Sats from the devfund. Submit an invoice with a long expiration time from a proxy node id :)

I agree, I will change the PR.

Merging! Thank you for your top contributions! 🚀

Don't forget the invoice :D Remember that some mobile wallets will be turned-off when they are in the background and cannot receive payments.

@Reckless-Satoshi Reckless-Satoshi merged commit 9a10077 into RoboSats:main May 26, 2023
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.

It's difficult to enter a discount because the field forces a zero into it
2 participants