PM-3260 points in wallet #1409
PM-3260 points in wallet #1409
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| /> | ||
|
|
||
| {!!props.pointsBalance && ( |
There was a problem hiding this comment.
[correctness]
The check !!props.pointsBalance will not render the InfoRow if pointsBalance is 0. If displaying a zero balance is intended, consider using props.pointsBalance !== undefined instead.
| const balanceSum = useMemo( | ||
| () => (walletDetails ? walletDetails.account.balances.reduce((sum, balance) => sum + balance.amount, 0) : 0), | ||
| const [paymentsBalance, pointsBalance] = useMemo( | ||
| () => ((walletDetails?.account.balances ?? []).reduce((sums, balance) => { |
There was a problem hiding this comment.
[💡 readability]
Consider using a more descriptive variable name for sums to improve readability, such as balances or totals.
| const date = new Date(iosDateString) | ||
|
|
||
| if (Number.isNaN(date.getTime())) { | ||
| throw new Error('Invalid date string') |
There was a problem hiding this comment.
[correctness]
Throwing an error for an invalid date string might not be the best approach if this function is used in contexts where invalid input is expected. Consider returning a default value or handling the error gracefully.
| case 'RETURNED': | ||
| return 'Returned' | ||
| default: | ||
| return status.replaceAll('_', ' ') |
There was a problem hiding this comment.
[correctness]
The use of replaceAll is not supported in all environments. Ensure that the target environment supports this method or consider using a more compatible approach like replace with a global regex.
| let amount: number | ||
| try { | ||
| amount = parseFloat(amountStr) | ||
| } catch (error) { |
There was a problem hiding this comment.
[💡 correctness]
The parseFloat function will not throw an error, so the try-catch block is unnecessary here. Consider removing the try-catch or handling potential issues differently.
| const fetchWinnings = useCallback(async () => { | ||
| setIsLoading(true) | ||
| try { | ||
| const payments = await getPayments(props.profile.userId.toString(), pagination.pageSize, (pagination.currentPage - 1) * pagination.pageSize, filters) |
There was a problem hiding this comment.
[maintainability]
Consider adding error handling for the getPayments call to provide more user-friendly feedback or retry logic in case of network issues.
| setSelectedPayments(selectedWinnings) | ||
| }} | ||
| onNextPageClick={async function onNextPageClicked() { | ||
| if (pagination.currentPage === pagination.totalPages) { |
There was a problem hiding this comment.
[💡 style]
The onNextPageClick function is marked as async but does not contain any await expressions. Consider removing async if not needed.
| currentPage: pagination.currentPage + 1, | ||
| }) | ||
| }} | ||
| onPreviousPageClick={async function onPrevPageClicked() { |
There was a problem hiding this comment.
[💡 style]
The onPreviousPageClick function is marked as async but does not contain any await expressions. Consider removing async if not needed.
| currentPage: pagination.currentPage - 1, | ||
| }) | ||
| }} | ||
| onPageClick={async function onPageClicked(pageNumber: number) { |
There was a problem hiding this comment.
[💡 style]
The onPageClick function is marked as async but does not contain any await expressions. Consider removing async if not needed.
| const date = new Date(iosDateString) | ||
|
|
||
| if (Number.isNaN(date.getTime())) { | ||
| throw new Error('Invalid date string') |
There was a problem hiding this comment.
[❗❗ correctness]
Throwing an error for an invalid date string might cause the application to crash if not handled properly. Consider handling this error gracefully or logging it instead.
| }) | ||
|
|
||
| const convertToPoints = useCallback( | ||
| (pointsData: any[]) => pointsData.map((point: any) => ({ |
There was a problem hiding this comment.
[maintainability]
Using any type for pointsData and point can lead to runtime errors due to lack of type safety. Consider defining a type or interface for the expected structure of pointsData.
| const fetchPoints = useCallback(async () => { | ||
| setIsLoading(true) | ||
| try { | ||
| const response = await getPoints(props.profile.userId.toString(), pagination.pageSize, (pagination.currentPage - 1) * pagination.pageSize, filters) |
There was a problem hiding this comment.
[❗❗ correctness]
Consider adding error handling for the getPoints function call to manage potential API failures more robustly.
| setHasPointsWinnings(pointsWinnings.length > 0) | ||
| }, [isLoading, pointsWinnings]) | ||
|
|
||
| if (!hasPointsWinnings) { |
There was a problem hiding this comment.
[💡 readability]
Returning an empty fragment when hasPointsWinnings is false might not provide any feedback to the user. Consider displaying a message indicating that no points are available.
| currentPage: 1, | ||
| } | ||
| if (key === 'pageSize') { | ||
| newPagination.pageSize = parseInt(value[0], 10) |
There was a problem hiding this comment.
[❗❗ correctness]
Parsing value[0] without checking if value is non-empty could lead to runtime errors. Consider adding a check to ensure value has at least one element.
| display: flex; | ||
| flex-direction: column; | ||
| gap: $sp-6; | ||
| background-color: $black-5; |
There was a problem hiding this comment.
[maintainability]
The background-color property is set twice for the .content class, first to $tc-white and then to $black-5. This could lead to confusion or unintended styling issues. Consider removing the redundant property to ensure clarity and maintainability.
|
|
||
| // Scroll to points section after a short delay to allow rendering | ||
| setTimeout(() => { | ||
| pointsRef.current?.scrollIntoView({ behavior: 'smooth', block: 'start' }) |
There was a problem hiding this comment.
[correctness]
Using setTimeout with a fixed delay for scrolling might lead to inconsistent behavior if rendering takes longer than expected. Consider using a useEffect with a dependency on the element's visibility or a callback ref to ensure scrolling occurs after the element is rendered.
| justify-content: center; | ||
| min-width: 32px; | ||
| padding: 0 0.5rem; | ||
| transition: all 0.2s; |
There was a problem hiding this comment.
[performance]
Using transition: all can lead to performance issues as it applies the transition to all properties, even those that don't need it. Consider specifying only the properties that require a transition, such as background-color and border-color.
| key={i} | ||
| type='button' | ||
| className={`${styles.paginationButton} ${i === props.currentPage ? styles.active : ''}`} | ||
| onClick={() => props.onPageClick(i)} |
There was a problem hiding this comment.
[performance]
Using arrow functions directly in the onClick handler can lead to unnecessary re-renders because a new function instance is created on each render. Consider defining the function outside the JSX to improve performance.
| <div className={styles.totalContainer}> | ||
| <span className='body-small'> | ||
| Total: | ||
| {props.points.length} |
There was a problem hiding this comment.
[💡 readability]
Consider adding a space after the 'Total:' label for better readability of the total points count.
|
|
||
| // eslint-disable-next-line max-len | ||
| export async function getPayments(userId: string, limit: number, offset: number, filters: Record<string, string[]>): Promise<{ | ||
| export async function getchUserWinnings( |
There was a problem hiding this comment.
[readability]
The function name getchUserWinnings appears to be a typo. It should likely be fetchUserWinnings or getUserWinnings for clarity and consistency.
|
|
||
| for (const key in filters) { | ||
| if (filters[key].length > 0 && key !== 'pageSize') { | ||
| if (filters[key].length > 0 && key !== 'pageSize' && filters[key][0]) { |
There was a problem hiding this comment.
[💡 readability]
The condition filters[key][0] is checked twice: once implicitly by filters[key].length > 0 and once explicitly. Consider simplifying this condition to improve readability.
| Dispatch<SetStateAction<boolean>> | ||
| ] = useState<boolean>(props.isCollapsed ?? false) | ||
|
|
||
| // Sync internal state with prop if controlled |
There was a problem hiding this comment.
[performance]
The useEffect dependency array includes props.isCollapsed, which is correct for syncing the internal state. However, ensure that props.isCollapsed is stable and not recreated on every render to avoid unnecessary effect executions.
| } | ||
| }, [props.isCollapsed]) | ||
|
|
||
| const isCollapsed = props.isCollapsed !== undefined ? props.isCollapsed : internalIsCollapsed |
There was a problem hiding this comment.
[💡 maintainability]
The logic for determining isCollapsed could be simplified by directly using internalIsCollapsed when props.isCollapsed is undefined. This would improve readability and maintainability.
|
|
||
| // eslint-disable-next-line max-len | ||
| export async function getPayments(userId: string, limit: number, offset: number, filters: Record<string, string[]>): Promise<{ | ||
| export async function getchUserWinnings( |
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-3260
What's in this PR?
Points visualization for members in their TC Wallet to allow hostory and tracking of earned points.