feat(TokenEnterAmount): Add new Enter Amount component #6242
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6242 +/- ##
==========================================
+ Coverage 88.92% 88.93% +0.01%
==========================================
Files 737 738 +1
Lines 31431 31513 +82
Branches 5837 5864 +27
==========================================
+ Hits 27950 28027 +77
+ Misses 3437 3287 -150
- Partials 44 199 +155
... and 69 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| const formattedInputValue = useMemo(() => { | ||
| const number = groupNumber(inputValue) | ||
| .replaceAll('.', decimalSeparator) | ||
| .replaceAll('group', groupingSeparator) | ||
| if (amountType === 'token') return number | ||
| return number !== '' ? `${localCurrencySymbol}${number}` : '' | ||
| }, [inputValue, amountType, localCurrencySymbol]) |
There was a problem hiding this comment.
I figured its easier to add currency sign before whatever is typed in the amount field rather than trying to parse it while having that sign and manage all the use-cases when it whether includes the sign (when typing in fiat) or not (when typing in crypto).
| } | ||
| } | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Kudos to @kathaypacific for implementing 95% of this component 🚀
| inputValue: '1', | ||
| tokenAmount: '1', | ||
| localAmount: '$0.1', | ||
| amountType: 'token' as AmountEnteredIn, |
There was a problem hiding this comment.
| amountType: 'token' as AmountEnteredIn, | |
| amountType: 'token' as const, |
There was a problem hiding this comment.
@kathaypacific considering this is more for the intellisense purpose I would keep it as is!
| toggleAmountType: mockToggleAmountType, | ||
| onOpenTokenPicker: mockOnOpenTokenPicker, | ||
| editable: true, | ||
| testID: 'tokenEnterAmount', |
There was a problem hiding this comment.
super nit: i think the pattern is to use TitleCase for test id's
| testID: 'tokenEnterAmount', | |
| testID: 'TokenEnterAmount', |
There was a problem hiding this comment.
@kathaypacific Sorry, this was the part generated by Chat GPT so I just kept it unchanged 🤦
Will change!
| testID: 'tokenEnterAmount', | ||
| } | ||
|
|
||
| it('renders without crashing', () => { |
There was a problem hiding this comment.
nit: i think that this test is unnecessary, the scenario is already covered by the following tests (since the input to the render method is the same, and asserting on elements already ensures that the component has successfully rendered without errors)
| <TokenEnterAmount {...defaultProps} /> | ||
| </Provider> | ||
| ) | ||
| expect(getByTestId('tokenEnterAmount/TokenName')).toBeTruthy() |
There was a problem hiding this comment.
| expect(getByTestId('tokenEnterAmount/TokenName')).toBeTruthy() | |
| expect(getByTestId('tokenEnterAmount/TokenName')).toHaveTextContent('CELO on Celo Alfajores') |
| expect(getByTestId('tokenEnterAmount/TokenSelect')).toBeTruthy() | ||
| expect(getByTestId('tokenEnterAmount/TokenBalance')).toBeTruthy() | ||
| expect(getByTestId('tokenEnterAmount/TokenBalance').props.children.props.i18nKey).toBe( | ||
| 'tokenEnterAmount.availableBalance' |
There was a problem hiding this comment.
if it's possible we should avoid digging into props since then we test implementation details rather than user outcomes...could instead consider doing something like
expect(getByTestId('tokenEnterAmount/TokenBalance')).toHaveTextContent(
'tokenEnterAmount.availableBalance'
)
expect(getByTestId('tokenEnterAmount/TokenBalance')).toHaveTextContent('5.00 CELO')
?
There was a problem hiding this comment.
@kathaypacific This makes sense! I've tried initially with getByText but for some reason for this particular test it couldn't find an element with that text (probably, cause it was including multiple nested Text tags so it wasn't able to match it). But your suggestion might work! Will try it out.
| const input = getByTestId('tokenEnterAmount/TokenAmountInput') | ||
| const converted = getByTestId('tokenEnterAmount/ExchangeAmount') | ||
| expect(input.props.value).toBe('1') | ||
| expect(converted.props.children).toBe(`${APPROX_SYMBOL} $0.1`) |
There was a problem hiding this comment.
| expect(converted.props.children).toBe(`${APPROX_SYMBOL} $0.1`) | |
| expect(converted).toHaveTextContent(`${APPROX_SYMBOL} $0.1`) |
| ) | ||
| const input = getByTestId('tokenEnterAmount/TokenAmountInput') | ||
|
|
||
| expect(input.props.editable).toBe(false) |
There was a problem hiding this comment.
| expect(input.props.editable).toBe(false) | |
| expect(input).toBeDisabled() |
| </Provider> | ||
| ) | ||
| const exchangeAmount = getByTestId('tokenEnterAmount/ExchangeAmount') | ||
| expect(exchangeAmount.props.children).toBe(`${APPROX_SYMBOL} $0.1`) |
There was a problem hiding this comment.
| expect(exchangeAmount.props.children).toBe(`${APPROX_SYMBOL} $0.1`) | |
| expect(exchangeAmount).toHaveTextContent(`${APPROX_SYMBOL} $0.1`) |
| const BORDER_RADIUS = 12 | ||
|
|
||
| function groupNumber(value: string) { | ||
| return value.replace(/\B(?=(\d{3})+(?!\d))(?<!\.\d*)/g, 'group') |
There was a problem hiding this comment.
can we please add a comment about what this regex does? 🙈 i would definitely benefit from this, i don't speak regex fluently
There was a problem hiding this comment.
@kathaypacific I've refactored this function a bit in the next PR and added comments explaining how it works!
| const formattedInputValue = useMemo(() => { | ||
| const number = groupNumber(inputValue) | ||
| .replaceAll('.', decimalSeparator) | ||
| .replaceAll('group', groupingSeparator) |
There was a problem hiding this comment.
could we add a couple of tests around the expected outcome of this formatting function? i think i may be missing something, but i'm surprised that we can freely use '.' here - if the inputValue is 1.234.567,888, would this function still work? (unsure if this is a real scenario and if consumers of this component will pass in formatted values, but this formatting convention is real)
There was a problem hiding this comment.
@kathaypacific you're right! It was a bug that I've revealed once I was working on a useEnterAmount hook in the next PR. I've refactored this part as well so it should be resolved there!
In order to not keep these issues in the codebase - I would wait for the next PR to also get approved and will merge them both in a batch!
Description
1/5 PR for new Enter Amount component. This PR implements a new component but doesn't use it anywhere. The tests are absent as this component will be covered with its usages in the follow-up PRs.
This is only done to reduce amount of lines to review per PR as at the end this whole feature is about 1k new lines and -1k of removed lines.
Test plan
Omit tests for now but in the follow-up PRs the following test files will be testing this component:
EnterAmount.test.tsxEarnEnterAmount.test.tsxSwapScreen.test.tsxThere will also be some test files that are gonna be fixed alongside. Please, see the follow-up PRs for more details.
Related issues
Backwards compatibility
Yes
Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: