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.

Sign up for GitHub

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

feat: add count up #1642

Merged
merged 6 commits into from
Nov 11, 2024
Merged

feat: add count up #1642

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5f33b40
feat: add count up
Nick-1979 Nov 11, 2024
7c856fe
refactor: rename var
Nick-1979 Nov 11, 2024
ac46aba
refactor: use decimal directly
Nick-1979 Nov 11, 2024
6dcfe4b
chore: add count up to extension mode
Nick-1979 Nov 11, 2024
4ab1e6f
chore: remove unused
Nick-1979 Nov 11, 2024
9726aa6
fix: change when selected currency is a crypto (#1643)
Nick-1979 Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
"memoize-one": "^6.0.0",
"qrcode.react": "^3.0.2",
"react-chartjs-2": "^5.2.0",
"react-countup": "^6.5.3",
"react-json-to-table": "^0.1.7",
"react-markdown": "^8.0.7",
"react-tooltip": "^4.2.21",
Expand Down
18 changes: 15 additions & 3 deletions packages/extension-polkagate/src/components/FormatPrice.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@

import { Grid, Skeleton, Typography } from '@mui/material';
import React, { useMemo } from 'react';
import CountUp from 'react-countup';

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';

Check failure on line 12 in packages/extension-polkagate/src/components/FormatPrice.tsx

View workflow job for this annotation

GitHub Actions / build 

'countDecimalPlaces' is declared but its value is never read.
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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.


interface Props {
amount?: BN | null;
Expand All @@ -27,6 +28,7 @@
textColor?: string;
height?: number;
width?: string;
withCountUp?: boolean;
}

export function nFormatter (num: number, decimalPoint: number) {
Expand Down Expand Up @@ -54,7 +56,7 @@

const DECIMAL_POINTS_FOR_CRYPTO_AS_CURRENCY = 4;

function FormatPrice ({ amount, commify, decimalPoint = 2, decimals, fontSize, fontWeight, height, lineHeight = 1, mt = '0px', num, price, sign, skeletonHeight = 15, textAlign = 'left', textColor, width = '90px' }: Props): React.ReactElement<Props> {
function FormatPrice ({ amount, commify, decimalPoint = 2, decimals, fontSize, fontWeight, height, lineHeight = 1, mt = '0px', num, price, sign, skeletonHeight = 15, textAlign = 'left', textColor, width = '90px', withCountUp }: Props): React.ReactElement<Props> {
const currency = useCurrency();

const total = useMemo(() => {
Expand Down Expand Up @@ -91,7 +93,17 @@
lineHeight={lineHeight}
sx={{ color: textColor }}
>
{sign || currency?.sign || ''}{ commify ? fixFloatingPoint(total as number, _decimalPoint, true) : nFormatter(total as number, _decimalPoint)}
{withCountUp
? <CountUp
decimals={_decimalPoint}
duration={1}
end={parseFloat(String(total))}
prefix={sign || currency?.sign || ''}
/>
Comment on lines +97 to +102
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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:

  1. 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>
  1. Respect user's motion preferences by utilizing the existing prefersReducedMotion check from useAnimateOnce.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:

  1. Respects user's reduced motion preferences
  2. Is properly announced by screen readers
  3. 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

: <>
{sign || currency?.sign || ''}{ commify ? fixFloatingPoint(total as number, _decimalPoint, true) : nFormatter(total as number, _decimalPoint)}
</>
}
</Typography>
: <Skeleton
animation='wave'
Expand Down
24 changes: 22 additions & 2 deletions packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type { FetchedBalance } from '../../../hooks/useAssetsBalances';
import { ArrowDropDown as ArrowDropDownIcon } from '@mui/icons-material';
import { Box, Collapse, Divider, Grid, type Theme, Typography, useTheme } from '@mui/material';
import React, { useCallback, useContext, useEffect, useMemo, useState } from 'react';
import CountUp from 'react-countup';

import { BN, BN_ZERO } from '@polkadot/util';

Expand All @@ -19,7 +20,7 @@ import { isPriceOutdated } from '../../../popup/home/YouHave';
import { COIN_GECKO_PRICE_CHANGE_DURATION } from '../../../util/api/getPrices';
import { DEFAULT_COLOR, TEST_NETS, TOKENS_WITH_BLACK_LOGO } from '../../../util/constants';
import getLogo2 from '../../../util/getLogo2';
import { amountToHuman, fixFloatingPoint } from '../../../util/utils';
import { amountToHuman, countDecimalPlaces, fixFloatingPoint } from '../../../util/utils';
import Chart from './Chart';

interface Props {
Expand Down Expand Up @@ -194,6 +195,16 @@ function TotalBalancePieChart ({ hideNumbers, setGroupedAssets }: Props): React.

const toggleAssets = useCallback(() => setShowMore(!showMore), [showMore]);

const portfolioChange = useMemo(() => {
if (!youHave?.change) {
return 0;
}

const value = fixFloatingPoint(youHave.change, 2, false, true);

return parseFloat(value);
}, [youHave?.change]);

return (
<Grid alignItems='flex-start' container direction='column' item justifyContent='flex-start' sx={{ bgcolor: 'background.paper', borderRadius: '5px', boxShadow: '2px 3px 4px 0px rgba(0, 0, 0, 0.1)', minHeight: '287px', p: '15px 25px 10px', width: '430px' }}>
<Grid alignItems='flex-start' container item justifyContent='flex-start'>
Expand All @@ -214,9 +225,18 @@ function TotalBalancePieChart ({ hideNumbers, setGroupedAssets }: Props): React.
fontWeight={700}
num={youHave?.portfolio}
textColor= { isPriceOutdated(youHave) ? 'primary.light' : 'text.primary'}
withCountUp
/>
<Typography sx={{ color: youHave.change > 0 ? 'success.main' : 'warning.main', fontSize: '18px', fontWeight: 500 }}>
{youHave.change > 0 ? '+ ' : '- '}{currency?.sign}{ fixFloatingPoint(youHave?.change, 2, true, true)} {`(${COIN_GECKO_PRICE_CHANGE_DURATION}h)`}
{portfolioChange &&
<CountUp
decimals={countDecimalPlaces(portfolioChange)}
duration={1}
end={portfolioChange}
prefix={`${youHave.change > 0 ? '+ ' : '- '}${currency?.sign}`}
suffix={`(${COIN_GECKO_PRICE_CHANGE_DURATION}h)`}
/>
}
</Typography>
</>
}
Expand Down
6 changes: 6 additions & 0 deletions packages/extension-polkagate/src/util/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ function countLeadingZerosInFraction (numStr: string) {
return 0;
}

export function countDecimalPlaces (n: number) {
const match = n.toString().match(/\.(\d+)/);

return match ? match[1].length : 0;
}
Comment on lines +53 to +57
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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
 */


export function fixFloatingPoint (_number: number | string, decimalDigit = FLOATING_POINT_DIGIT, commify?: boolean, dynamicDecimal?: boolean): string {
const MAX_DECIMAL_POINTS = 6;

Expand Down
19 changes: 19 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8297,6 +8297,13 @@ __metadata:
languageName: node
linkType: hard

"countup.js@npm:^2.8.0":
version: 2.8.0
resolution: "countup.js@npm:2.8.0"
checksum: 10/6681de9de9acd0d65b91183af280a462f2fbbc19ac5a90d30ea94e388f12b0d1cc7fa2b63b6bb83adc9dc38f938cd2a41f727a0b2ae7edf0d82f4b2ccb51e98c
languageName: node
linkType: hard

"crc-32@npm:^1.2.2":
version: 1.2.2
resolution: "crc-32@npm:1.2.2"
Expand Down Expand Up @@ -16694,6 +16701,17 @@ __metadata:
languageName: node
linkType: hard

"react-countup@npm:^6.5.3":
version: 6.5.3
resolution: "react-countup@npm:6.5.3"
dependencies:
countup.js: "npm:^2.8.0"
peerDependencies:
react: ">= 16.3.0"
checksum: 10/5699e475bdd82b0b100174acbe24a33c9025080bacbba9f28593308145eacfb3e0d30be5de5bb153121346b719375e317a9f2aea87776f38d508cbe769ac0d8b
languageName: node
linkType: hard

"react-dom@npm:^16.7.0":
version: 16.14.0
resolution: "react-dom@npm:16.14.0"
Expand Down Expand Up @@ -17620,6 +17638,7 @@ __metadata:
pinst: "npm:^3.0.0"
qrcode.react: "npm:^3.0.2"
react-chartjs-2: "npm:^5.2.0"
react-countup: "npm:^6.5.3"
react-json-to-table: "npm:^0.1.7"
react-markdown: "npm:^8.0.7"
react-tooltip: "npm:^4.2.21"
Expand Down
Loading