feat: add count up #1642
feat: add count up #1642
Conversation
|
Warning Rate limit exceeded@Nick-1979 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces a new dependency, Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/extension-polkagate/src/components/FormatPrice.tsx (2)
82-86: Consider adding error handling for invalid numbersWhile the number formatting logic is correct, it might benefit from additional error handling for edge cases.
Consider adding null/undefined checks and handling NaN:
const no = useMemo(() => { + if (typeof total !== 'number' || isNaN(total)) { + return 0; + } const temp = fixFloatingPoint(total as number, _decimalPoint); return parseFloat(temp); }, [_decimalPoint, total]);
102-112: Consider making CountUp animation duration configurableThe CountUp implementation is good, but the animation duration is hardcoded to 1 second. Consider making it configurable through props for more flexibility.
Add a duration prop and apply it to CountUp:
interface Props { // ... existing props + animationDuration?: number; } function FormatPrice ({ // ... existing props + animationDuration = 1, }: Props) { // ... in the render section <CountUp decimals={countDecimalPlaces(no)} - duration={1} + duration={animationDuration} end={no} prefix={sign || currency?.sign || ''} />packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx (2)
198-206: Consider explicit null/undefined check for youHave.change.The current condition
if (!youHave?.change)will evaluate to true for both undefined/null and zero values. This might skip valid zero changes.Consider this more explicit check:
-if (!youHave?.change) { +if (youHave?.change === undefined || youHave?.change === null) {
231-239: Consider extracting animation duration to constants.The CountUp implementation is good, but the duration value is hardcoded. For consistency across the application, consider moving it to a constants file.
Example:
+// In constants.ts +export const NUMBER_ANIMATION_DURATION = 1; -duration={1} +duration={NUMBER_ANIMATION_DURATION}packages/extension-polkagate/src/util/utils.ts (1)
53-57: Consider enhancing integration with existing utilities.The new
countDecimalPlacesfunction could be utilized to improve the dynamic decimal handling infixFloatingPoint. This would provide consistent decimal place handling across the codebase.Consider refactoring
fixFloatingPointto use this function:export function fixFloatingPoint (_number: number | string, decimalDigit = FLOATING_POINT_DIGIT, commify?: boolean, dynamicDecimal?: boolean): string { const MAX_DECIMAL_POINTS = 6; // make number positive if it is negative const sNumber = Number(_number) < 0 ? String(-Number(_number)) : String(_number); const dotIndex = sNumber.indexOf('.'); if (dotIndex < 0) { return sNumber; } let integerDigits = sNumber.slice(0, dotIndex); if (integerDigits === '0' && dynamicDecimal) { // to show very small numbers such as 0.0000001123 - const leadingZerosInFraction = countLeadingZerosInFraction(sNumber); + const decimalPlaces = countDecimalPlaces(Number(sNumber)); + const leadingZeros = countLeadingZerosInFraction(sNumber); - if (leadingZerosInFraction > 0 && leadingZerosInFraction < MAX_DECIMAL_POINTS) { - decimalDigit = leadingZerosInFraction + 1; + if (leadingZeros > 0 && decimalPlaces <= MAX_DECIMAL_POINTS) { + decimalDigit = decimalPlaces; } }This change would make the decimal place handling more consistent and potentially more accurate for edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
package.json(1 hunks)packages/extension-polkagate/src/components/FormatPrice.tsx(5 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx(4 hunks)packages/extension-polkagate/src/util/utils.ts(1 hunks)
🔇 Additional comments (6)
packages/extension-polkagate/src/components/FormatPrice.tsx (3)
8-8: LGTM: Import statements are properly organized
The new imports for CountUp and utility functions are correctly placed and organized.
Also applies to: 12-12
31-31: LGTM: Props interface update is well-defined
The new withCountUp prop is properly typed as an optional boolean.
59-59: Verify consistent usage of CountUp across the codebase
Let's ensure that the CountUp feature is being used consistently in similar price/number display scenarios across the application.
✅ Verification successful
Based on the search results, I can now generate the final response:
CountUp animation is an optional enhancement that maintains compatibility
The withCountUp prop is an optional feature in the FormatPrice component, allowing gradual adoption across the codebase. The component is widely used across many UI components (20+ files), but the animation is not forced, maintaining backward compatibility. The implementation correctly handles both animated and static number displays through the same interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other price formatting components that might benefit from CountUp
rg -l "FormatPrice|CountUp" --type tsx
Length of output: 68
Script:
#!/bin/bash
# Search for FormatPrice and CountUp usage with ripgrep, using TypeScript file extensions
rg -l "FormatPrice|CountUp" -g "*.ts" -g "*.tsx"
# Get the implementation details of FormatPrice component to understand CountUp usage
rg "CountUp" -A 5 -B 5 packages/extension-polkagate/src/components/FormatPrice.tsx
Length of output: 3263
Script:
#!/bin/bash
# Let's check how CountUp is being used in other components
rg "withCountUp|CountUp" -A 3 -B 3 packages/extension-polkagate/src/popup/home/YouHave.tsx packages/extension-polkagate/src/popup/account/LabelBalancePrice.tsx packages/extension-polkagate/src/fullscreen/stake/partials/DisplayBalance.tsx
Length of output: 239
Script:
#!/bin/bash
# Search for actual usage of FormatPrice component with withCountUp prop
rg "FormatPrice.*withCountUp" -A 2 -B 2
# Also check for direct imports and usage of FormatPrice
rg "import.*FormatPrice" -A 5
Length of output: 15452
package.json (1)
84-84: LGTM! The addition of react-countup is appropriate.
The package is well-suited for implementing animated number displays in the FormatPrice and TotalBalancePieChart components. It's a stable, widely-used package with good performance characteristics.
Let's verify the implementation details:
✅ Verification successful
Implementation of react-countup is correctly done and no issues found
The verification confirms:
- CountUp is properly implemented in both FormatPrice and TotalBalancePieChart components
- No duplicate number animation libraries are present
- The implementation includes proper decimal handling and formatting
- Usage is consistent with best practices:
- Optional via
withCountUpprop in FormatPrice - Proper duration and decimal configuration
- Appropriate prefix/suffix handling for currency display
- Optional via
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify react-countup implementation and potential issues
# Test 1: Check if FormatPrice and TotalBalancePieChart components are using react-countup
rg -A 5 "import.*CountUp.*from 'react-countup'"
# Test 2: Verify no duplicate number animation libraries
rg -l "import.*from '@?[a-zA-Z-]+/count|number-animation'"
Length of output: 1494
Script:
#!/bin/bash
# Let's verify the implementation details more thoroughly
# Check how CountUp is being used in FormatPrice
rg -B 5 -A 10 "CountUp" packages/extension-polkagate/src/components/FormatPrice.tsx
# Check how CountUp is being used in TotalBalancePieChart
rg -B 5 -A 10 "CountUp" packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx
# Check for any other number animation related implementations
rg -l "animate.*number|number.*animation"
Length of output: 4174
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx (2)
11-11: LGTM: Import statements are properly organized.
The new imports for CountUp and countDecimalPlaces align with the PR objectives for adding animated number displays.
Also applies to: 23-23
228-228: LGTM: FormatPrice integration looks good.
The withCountUp prop addition to FormatPrice enables consistent number animation across the application.
| export function countDecimalPlaces (n: number) { | ||
| const match = n.toString().match(/\.(\d+)/); | ||
|
|
||
| return match ? match[1].length : 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety and handle edge cases.
The function implementation could be improved with better type safety and edge case handling:
-export function countDecimalPlaces (n: number) {
+export function countDecimalPlaces (n: number): number {
+ if (!Number.isFinite(n)) {
+ return 0;
+ }
+
+ // Handle scientific notation
+ const str = n.toString().toLowerCase();
+ if (str.includes('e')) {
+ const [_, exp] = str.split('e');
+ return Math.max(0, -Number(exp));
+ }
+
const match = n.toString().match(/\.(\d+)/);
return match ? match[1].length : 0;
}Also consider adding JSDoc documentation to describe the function's purpose and behavior:
/**
* Counts the number of decimal places in a given number.
* @param n - The number to analyze
* @returns The count of decimal places (0 for integers, NaN, or Infinity)
* @example
* countDecimalPlaces(1.23) // returns 2
* countDecimalPlaces(1) // returns 0
* countDecimalPlaces(1e-10) // returns 10
*/
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/extension-polkagate/src/components/FormatPrice.tsx (2)
97-102: Consider enhancing CountUp implementationA few suggestions to improve the CountUp integration:
- The
commifyprop is ignored when using CountUp- Consider making the animation duration configurable via props
- Add error handling for invalid number parsing
<CountUp decimals={_decimalPoint} - duration={1} + duration={props.duration ?? 1} + separator={commify ? ',' : ''} end={parseFloat(String(total))} + onError={(error) => console.warn('CountUp error:', error)} prefix={sign || currency?.sign || ''} />
96-106: Optimize component renderingThe conditional rendering could be optimized to prevent unnecessary re-renders by extracting the JSX into separate components or memoized values.
-{withCountUp - ? <CountUp - decimals={_decimalPoint} - duration={1} - end={parseFloat(String(total))} - prefix={sign || currency?.sign || ''} - /> - : <> - {sign || currency?.sign || ''}{ commify ? fixFloatingPoint(total as number, _decimalPoint, true) : nFormatter(total as number, _decimalPoint)} - </> -} +{useMemo(() => { + const prefix = sign || currency?.sign || ''; + return withCountUp ? ( + <CountUp + decimals={_decimalPoint} + duration={1} + end={parseFloat(String(total))} + prefix={prefix} + /> + ) : ( + <> + {prefix}{ commify ? fixFloatingPoint(total as number, _decimalPoint, true) : nFormatter(total as number, _decimalPoint)} + </> + ); +}, [withCountUp, _decimalPoint, total, sign, currency?.sign, commify])}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/extension-polkagate/src/components/FormatPrice.tsx(4 hunks)packages/extension-polkagate/src/popup/home/YouHave.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: build
packages/extension-polkagate/src/components/FormatPrice.tsx
[failure] 12-12:
'countDecimalPlaces' is declared but its value is never read.
🔇 Additional comments (3)
packages/extension-polkagate/src/popup/home/YouHave.tsx (2)
59-61: LGTM: Font size adjustment
The font size reduction to 32px aligns well with the component's height of 36px, providing better visual balance.
66-66: Verify FormatPrice implementation of withCountUp
The addition of withCountUp introduces animated number display functionality. Let's verify the FormatPrice component's implementation to ensure proper handling.
✅ Verification successful
FormatPrice implementation of withCountUp is properly handled
The FormatPrice component correctly implements the withCountUp prop, using react-countup for animated number display when enabled. The implementation:
- Accepts
withCountUpas an optional boolean prop - When
withCountUpis true, renders a CountUp component with proper configuration (decimals, duration, prefix) - When false or undefined, falls back to standard number formatting
- Properly integrates with other component features like currency signs and decimal points
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify FormatPrice component implementation of withCountUp prop
# Check FormatPrice component implementation
echo "Checking FormatPrice component implementation..."
ast-grep --pattern 'export const FormatPrice = ({ withCountUp, $$$' || \
ast-grep --pattern 'export default function FormatPrice({ withCountUp, $$$'
# Check for other usages of withCountUp prop
echo "Checking other usages of withCountUp prop..."
rg "withCountUp" --type ts --type tsx
# Check for react-countup imports
echo "Checking react-countup usage..."
rg "from ['|\"]react-countup['|\"]" --type ts --type tsx
Length of output: 505
Script:
#!/bin/bash
# Let's try a different approach without file type restrictions
# Check FormatPrice component implementation
echo "Checking FormatPrice component implementation..."
rg -l "FormatPrice"
# Check for withCountUp usage
echo "Checking withCountUp usage..."
rg "withCountUp"
# Check for react-countup imports
echo "Checking react-countup usage..."
rg "from ['\"]react-countup['\"]"
Length of output: 2854
Script:
#!/bin/bash
# Let's check the FormatPrice implementation to see how withCountUp is handled
echo "Checking FormatPrice implementation..."
cat packages/extension-polkagate/src/components/FormatPrice.tsx
Length of output: 3550
packages/extension-polkagate/src/components/FormatPrice.tsx (1)
31-31: LGTM: Props interface update is well-defined
The new withCountUp prop is properly typed as an optional boolean.
|
|
||
| import { useCurrency } from '../hooks'; | ||
| import { ASSETS_AS_CURRENCY_LIST } from '../util/currencyList'; | ||
| import { amountToHuman, fixFloatingPoint } from '../util/utils'; | ||
| import { amountToHuman, countDecimalPlaces, fixFloatingPoint } from '../util/utils'; |
There was a problem hiding this comment.
Remove unused import countDecimalPlaces
The countDecimalPlaces utility is imported but never used in this component.
-import { amountToHuman, countDecimalPlaces, fixFloatingPoint } from '../util/utils';
+import { amountToHuman, fixFloatingPoint } from '../util/utils';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { amountToHuman, countDecimalPlaces, fixFloatingPoint } from '../util/utils'; | |
| import { amountToHuman, fixFloatingPoint } from '../util/utils'; |
🧰 Tools
🪛 GitHub Check: build
[failure] 12-12:
'countDecimalPlaces' is declared but its value is never read.
| ? <CountUp | ||
| decimals={_decimalPoint} | ||
| duration={1} | ||
| end={parseFloat(String(total))} | ||
| prefix={sign || currency?.sign || ''} | ||
| /> |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response regarding the accessibility concerns of the CountUp component.
Add accessibility attributes to CountUp implementation
The current CountUp implementation lacks proper accessibility considerations. While the codebase shows awareness of reduced motion preferences in other components, the numeric animation needs similar treatment. Please update the implementation to:
- Wrap CountUp in a container with appropriate ARIA attributes:
<span aria-atomic="true" aria-live="polite">
<CountUp
decimals={_decimalPoint}
duration={prefersReducedMotion ? 0 : 1}
end={parseFloat(String(total))}
prefix={sign || currency?.sign || ''}
/>
</span>- Respect user's motion preferences by utilizing the existing
prefersReducedMotioncheck fromuseAnimateOnce.ts
This will ensure:
- Screen readers announce the final value properly
- Users with motion sensitivity preferences are respected
- The numeric value remains accessible during animation
🔗 Analysis chain
Verify accessibility impact
Please ensure that the CountUp animation:
- Respects user's reduced motion preferences
- Is properly announced by screen readers
- Doesn't cause content layout shifts
Let me check the content of these files to understand the existing accessibility implementations.
Let me check if there are any existing ARIA attributes or accessibility considerations in the FormatPrice component, and also check the react-countup package usage.
Let me check the TotalBalancePieChart component to see how CountUp is implemented there, and also verify the package.json for the react-countup version and any related accessibility packages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing accessibility considerations in the codebase
rg -l 'prefers-reduced-motion|aria-live|aria-atomic'
Length of output: 170
Script:
#!/bin/bash
# Check content of files with accessibility considerations
echo "=== VaadinIcon.tsx ==="
cat packages/extension-polkagate/src/components/VaadinIcon.tsx
echo -e "\n=== useAnimateOnce.ts ==="
cat packages/extension-polkagate/src/hooks/useAnimateOnce.ts
# Also check if CountUp component has any accessibility considerations
rg -A 5 "CountUp" packages/extension-polkagate/src/components/FormatPrice.tsx
Length of output: 4728
Script:
#!/bin/bash
# Check for any ARIA attributes in FormatPrice
rg -A 2 'aria-|role=' packages/extension-polkagate/src/components/FormatPrice.tsx
# Check if react-countup is used elsewhere with accessibility considerations
rg -l "react-countup"
# Look for any test files related to FormatPrice
fd "FormatPrice.*.test" || fd "FormatPrice.*.spec"
Length of output: 307
Script:
#!/bin/bash
# Check CountUp implementation in TotalBalancePieChart
cat packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx
# Check package.json for react-countup version and accessibility-related packages
cat package.json | grep -A 5 -B 5 "react-countup"
Length of output: 11484
# [0.27.0](v0.26.1...v0.27.0) (2024-11-11) ### Features * add count up ([#1642](#1642)) ([b155aaf](b155aaf))
Summary by CodeRabbit
New Features
withCountUpfeature.Bug Fixes
Chores
react-countup.