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

Improve exchange rate calculation #70

Conversation

reneaaron
Copy link
Contributor

@reneaaron reneaaron commented Apr 25, 2022

I did test it with a waveshare2in13v2 display, other displays unfortunately remain untested.

@reneaaron reneaaron marked this pull request as ready for review April 25, 2022 17:04
@AxelHamburch
Copy link
Contributor

Nice, its working! I think it will help to calculate a more accurate exchange rate.

I was a bit confused by the decimal place in the exchange rate. Especially since he sometimes shows one or two. Maybe one would be enough.

The first picture shows a calculation without the changes and the other two with the change.

ZomboDroid 27042022220510
.

@AxelHamburch
Copy link
Contributor

I don't think it's related, but during the tests I saw an error message in the debugger. However, it came with and without the change. That's why I'm more likely to assume that it was the lntxbot again. But for the sake of completeness I would like to mention it.
2022-04-27 21_18_26-pi@raspberrypi_ ~

@reneaaron
Copy link
Contributor Author

Thanks for testing and your feedback @AxelHamburch! 🙌

I agree that space is very limited and we could just displace one decimal place as this is just an additional info for the user. The final amount will be calculated with the exact exchange rate, anyways. I'll update the MR.

@AxelHamburch
Copy link
Contributor

Great! What do you think @lightisfaster ?

@reneaaron
Copy link
Contributor Author

@AxelHamburch I still need to incorporate your feedback. 😅 I'll have the MR ready later this week.

@reneaaron reneaaron force-pushed the feature/improve-exchange-rate-calculation branch from c85b966 to 01af775 Compare May 3, 2022 09:28
@reneaaron
Copy link
Contributor Author

@AxelHamburch I just updated the MR. Should be ready to merge!

@lightisfaster
Copy link
Collaborator

Hey, looks good for me, i've test it too. Thanks for feedback @AxelHamburch. Good job @reneaaron and sorry for the delay. The alignment of the text may need to be adjusted for a display type, but this is trade off we can accept.

@lightisfaster lightisfaster merged commit ec3a565 into 21isenough:master May 6, 2022
@reneaaron reneaaron deleted the feature/improve-exchange-rate-calculation branch May 7, 2022 19:58
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