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

Donation flow more fixes #1991

Merged
merged 14 commits into from
Dec 11, 2024

Conversation

sashko9807
Copy link
Member

No description provided.

Copy link

github-actions bot commented Dec 6, 2024

✅ Tests will run for this PR. Once they succeed it can be merged.

@sashko9807 sashko9807 added the run tests Allows running the tests workflows for forked repos label Dec 6, 2024
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Dec 6, 2024
@sashko9807 sashko9807 force-pushed the donation-flow-more-fixes branch from 8b9efe9 to 813875c Compare December 9, 2024 20:02
@sashko9807 sashko9807 added the run tests Allows running the tests workflows for forked repos label Dec 9, 2024
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Dec 9, 2024
Copy link
Contributor

@gparlakov gparlakov left a comment

Choose a reason for hiding this comment

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

LGTM. I've one question - see in comments

totalAmount: moneyPublicDecimals2(amountWithFees.value),
}}
/>
{!!amountWithFees.value && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to assertain that the value is a non-zero number? As in !isNaN(amountWithFees.value) && amountWithFees.value > 0

It's hard to read. And also negative numbers give true value as well => !!-1 === true

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is to show the text from below only if amountWithFees.value is truthy value. The double negation is needed in order to convert the amountWithFees.value to boolean, otherwise if we just pass amountWithFees.value it would display 0 in the DOM.

@sashko9807 sashko9807 added the run tests Allows running the tests workflows for forked repos label Dec 11, 2024
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Dec 11, 2024
@gparlakov
Copy link
Contributor

gparlakov commented Dec 11, 2024 via email

@sashko9807
Copy link
Member Author

Yes I get that. What I'm saying is 1 it is unreadable, and 2 it will show the value when it's negative number or a string or an object etc.

Abstracted the condition into its own variable(should be more readable now). As for the other concerns, they aren't really a problem, as the otherAmount field can't accept anything other than positive numbers.

@sashko9807 sashko9807 added the run tests Allows running the tests workflows for forked repos label Dec 11, 2024
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Dec 11, 2024
@sashko9807 sashko9807 added the run tests Allows running the tests workflows for forked repos label Dec 11, 2024
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Dec 11, 2024
@sashko9807 sashko9807 merged commit c26d6d2 into podkrepi-bg:master Dec 11, 2024
11 of 12 checks passed
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.

2 participants