Skip to content
This repository was archived by the owner on Dec 7, 2022. It is now read-only.

fix: adjust vault negative amounts #77

Merged
merged 2 commits into from
Feb 9, 2022
Merged

fix: adjust vault negative amounts #77

merged 2 commits into from
Feb 9, 2022

Conversation

samsiegart
Copy link
Contributor

@samsiegart samsiegart commented Feb 9, 2022

fixes: #248

Instead of crashing/throwing an app-level error, show an invalid input with a message indicating the locked/debt amount to subtract exceeds the current locked/debt amount

Also tweaked the cancel button so that it didn't look like a primary action

Screenshot:
image

@github-actions
Copy link

github-actions bot commented Feb 9, 2022

@@ -75,6 +76,8 @@ const PurseAmountInput = ({
decimalPlaces={decimalPlaces}
placesToShow={placesToShow}
disabled={amountInputDisabled}
error={error !== null}
helperText={error ?? ' '}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There may be a need for non-error helper text for a different feature, but that can be handled separately when needed.

The ' ' is a hack to prevent the height of the input from changing (and moving around the UI) when the helperText appears and disappears.

Copy link
Member

Choose a reason for hiding this comment

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

Is ?? in standard JS yet? Or is that transformed by our bundler? Just curious, I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let debtInput = component.find(NatPurseAmountInput).at(1);
expect(debtInput.props().error).toEqual(null);

act(() => debtInput.props().onAmountChange(0n));
Copy link
Contributor Author

@samsiegart samsiegart Feb 9, 2022

Choose a reason for hiding this comment

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

Technically this is only testing that it shows an error when AmountMath.subtract throws (it's mocked above), not anything to do with the actual amount values used, but we need to mock AmountMath since it requires ses which is incompatible with jest jestjs/jest#11952

@samsiegart samsiegart requested a review from dckc February 9, 2022 18:39
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

testing looks good.

Comment on lines +188 to +190
let newAmount;
try {
newAmount = AmountMath.subtract(locked, collDelta);
Copy link
Member

Choose a reason for hiding this comment

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

Changing const newAmount to let newAmount just because JS doesn't have try expressions but only statements always bugs me. But oh well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I could avoid it by stuffing more into the try-block, but I think it's better to isolate the error as specifically as possible

@@ -75,6 +76,8 @@ const PurseAmountInput = ({
decimalPlaces={decimalPlaces}
placesToShow={placesToShow}
disabled={amountInputDisabled}
error={error !== null}
helperText={error ?? ' '}
Copy link
Member

Choose a reason for hiding this comment

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

Is ?? in standard JS yet? Or is that transformed by our bundler? Just curious, I suppose.

@samsiegart samsiegart merged commit 89bcb19 into main Feb 9, 2022
@samsiegart samsiegart deleted the negative-amounts branch February 9, 2022 20:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants