style: add effect to favorite chain and currency selection #1656
Conversation
chore: remove unused
…ate/extension into addEffectToFavoriateChain
WalkthroughThe changes in this pull request involve significant modifications to several components within the application, primarily focusing on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 4
🧹 Outside diff range and nitpick comments (10)
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/Currency.tsx (2)
Line range hint
28-35: Consider adding a loading state for better UXThe initial currency loading might cause a flash of default content ('$' sign) before the stored currency loads.
Consider adding a loading state:
const [currencyToShow, setCurrencyToShow] = useState<CurrencyItemType | undefined>(); +const [isLoading, setIsLoading] = useState(true); useLayoutEffect(() => { if (currencyToShow) { return; } getStorage('currency').then((res) => { setCurrencyToShow((res as CurrencyItemType)); + setIsLoading(false); }).catch(console.error); }, [currencyToShow]);
57-87: Improve Dialog positioning and stylingThe current implementation has several potential issues:
- Hard-coded positioning values could cause layout issues on different screen sizes
- Backdrop styling could be moved to theme for consistency
Consider these improvements:
<Dialog PaperProps={{ sx: { bgcolor: 'background.paper', borderRadius: '7px', boxShadow: theme.palette.mode === 'dark' ? '0px 4px 4px rgba(255, 255, 255, 0.25)' : '0px 0px 25px 0px rgba(0, 0, 0, 0.50)', - left: anchorEl?.getBoundingClientRect().right - 300, - position: 'absolute', - top: anchorEl?.getBoundingClientRect().bottom - 45 + position: 'fixed', + maxWidth: '300px', + width: '100%', + transform: `translate( + ${Math.max(0, Math.min(anchorEl?.getBoundingClientRect().right - 300, window.innerWidth - 300))}px, + ${Math.max(0, Math.min(anchorEl?.getBoundingClientRect().bottom - 45, window.innerHeight - 400))}px + )` } }} TransitionComponent={Slide} onClose={handleClose} open={!!anchorEl} slotProps={{ backdrop: { - sx: { - backdropFilter: 'blur(8px)', - backgroundColor: 'rgba(0, 0, 0, 0.3)' - } + sx: theme.components?.MuiBackdrop?.styleOverrides?.root ?? { + backdropFilter: 'blur(8px)', + backgroundColor: 'rgba(0, 0, 0, 0.3)' + } } }} >packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/FavoriteChains.tsx (1)
16-16: Consider removing unused interface.The
CurrencyItemTypeinterface appears to be unused in this file since it's focused on chain management rather than currency.packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyList.tsx (4)
23-28: Consider adding JSDoc documentation.Since the component has been renamed from
CurrencySwitchtoCurrencyList, it would be helpful to add JSDoc documentation describing its purpose and functionality.+/** + * CurrencyList component provides a searchable list of currencies for selection. + * It allows users to filter and select their preferred currency for balance display. + */ function CurrencyList ({ anchorEl, setAnchorEl, setCurrencyToShow }: Props): React.ReactElement {
29-33: Consider memoizing the cleanup callback.The effect's cleanup logic could be memoized using useCallback to optimize performance.
+const clearSearch = useCallback(() => { + setSearchKeyword(''); +}, []); useEffect(() => { if (anchorEl === null) { - setSearchKeyword(''); + clearSearch(); } }, [anchorEl, clearSearch]);
Line range hint
35-41: Improve error handling for storage operation.The storage error is currently only logged to console. Consider handling the error more gracefully or notifying the user.
const changeCurrency = useCallback((currency: CurrencyItemType) => { setAnchorEl(null); setCurrency(currency); setCurrencyToShow(currency); - setStorage('currency', currency).catch(console.error); + setStorage('currency', currency).catch((error) => { + console.error('Failed to save currency preference:', error); + // Consider showing a notification to the user + }); }, [setAnchorEl, setCurrency, setCurrencyToShow]);
Line range hint
43-60: Optimize search performance.The current implementation creates a new array on each filter operation and could be optimized.
+const filterCurrencies = useCallback((keyword: string) => + CURRENCY_LIST.filter((currency) => + currency.code.toLowerCase().includes(keyword) || + currency.country.toLowerCase().includes(keyword) || + currency.currency.toLowerCase().includes(keyword) + ), []); const onSearch = useCallback((keyword: string) => { if (!keyword) { setSearchKeyword(''); return setSearchedCurrency(undefined); } setSearchKeyword(keyword); keyword = keyword.trim().toLowerCase(); - const filtered = CURRENCY_LIST.filter((currency) => - currency.code.toLowerCase().includes(keyword) || - currency.country.toLowerCase().includes(keyword) || - currency.currency.toLowerCase().includes(keyword) - ); - setSearchedCurrency([...filtered]); + setSearchedCurrency(filterCurrencies(keyword)); }, [filterCurrencies]);packages/extension-polkagate/src/fullscreen/homeFullScreen/components/ChainList.tsx (3)
134-134: Consider using responsive width instead of fixed width.The fixed width of 280px might not be optimal for all screen sizes. Consider using responsive units or Grid's breakpoint system.
-<Grid container item sx={{ width: '280px' }}> +<Grid container item sx={{ width: { xs: '100%', sm: '280px' } }}>
158-163: Review accessibility and UX of the search input.Two suggestions for improvement:
- The
autoFocusprop might interfere with keyboard navigation and accessibility. Consider removing it unless absolutely necessary.- Using emoji in placeholder text isn't recommended for accessibility. Consider using an icon component instead.
<InputFilter - autoFocus onChange={onSearch} - placeholder={t<string>('🔍 Search chain')} + placeholder={t<string>('Search chain')} + startAdornment={<SearchIcon />} theme={theme} value={searchKeyword ?? ''} />
Line range hint
166-176: Optimize list rendering and transitions.Several improvements can be made to enhance performance and user experience:
- The transition duration of 5000ms (5 seconds) is too long for a smooth user experience
- Using array index as key in the map function can lead to performance issues and unexpected behavior when items are reordered
- Consider using virtualization for better performance with large lists
-<Grid container item sx={{ maxHeight: 'calc(100vh - 200px)', overflow: 'hidden', overflowY: 'scroll', transition: 'height 5000ms ease-in-out' }}> +<Grid container item sx={{ maxHeight: 'calc(100vh - 200px)', overflow: 'hidden', overflowY: 'auto', transition: 'height 300ms ease-in-out' }}> {[...(searchedChain ?? sortedChainsToShow)].map((item, index) => ( <ChainItem chain={item} disabled={!isTestnetEnabled && TEST_NETS.includes(item.value as string)} isSelected={selectedChains.has(item.value as string)} - key={index} + key={item.value} onclick={onChainSelect} /> ))} </Grid>Consider using a virtualized list component like
react-windowfor better performance with large lists:import { FixedSizeList } from 'react-window'; const Row = ({ data, index, style }) => { const item = data[index]; return ( <div style={style}> <ChainItem chain={item} disabled={!isTestnetEnabled && TEST_NETS.includes(item.value as string)} isSelected={selectedChains.has(item.value as string)} onclick={onChainSelect} /> </div> ); }; // In render: <FixedSizeList height={window.innerHeight - 200} itemCount={items.length} itemSize={50} width="100%" > {Row} </FixedSizeList>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/extension-polkagate/src/fullscreen/homeFullScreen/components/ChainList.tsx(4 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyList.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/Currency.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/FavoriteChains.tsx(2 hunks)packages/extension/package.json(1 hunks)packages/extension/src/packageInfo.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/extension/package.json
- packages/extension/src/packageInfo.ts
🔇 Additional comments (7)
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/Currency.tsx (1)
Line range hint 20-91: Implementation aligns well with PR objectives
The transition from Popover to Dialog and the added effects enhance the currency selection experience. The component maintains good React practices with proper state management and memoization.
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/FavoriteChains.tsx (2)
87-93: LGTM! Nice backdrop effect.
The blur effect on the backdrop provides good visual feedback and aligns with the PR objective of adding effects to the chain selection.
90-91: Verify backdrop blur performance.
While the blur effect looks good, it might impact performance on lower-end devices.
✅ Verification successful
Backdrop blur usage is consistent with existing patterns
The blur effect (8px) is already used consistently across similar components in the codebase, particularly in modal/overlay scenarios. The implementation follows the established pattern where:
- Home screen components use 8px blur with 0.3 opacity
- Governance components use 5px blur with 0.4 opacity
Since this is an established pattern in the codebase and used in limited overlay scenarios, the performance impact should be acceptable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other backdrop blur implementations to ensure consistent usage
rg -l 'backdropFilter.*blur' --type ts --type tsx
Length of output: 79
Script:
#!/bin/bash
# Search for backdrop blur implementations with correct file type patterns
rg -l 'backdropFilter.*blur' -g '*.ts' -g '*.tsx'
Length of output: 373
Script:
#!/bin/bash
# Check the blur values and implementation details in these files
rg 'backdropFilter.*blur' -g '*.ts' -g '*.tsx' -B 2 -A 2
Length of output: 2302
packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyList.tsx (2)
Line range hint 8-22: LGTM! Clean imports and well-typed props interface.
The component's dependencies and type definitions are properly organized and typed.
93-93: LGTM! Appropriate use of React.memo.
The component is correctly memoized to prevent unnecessary re-renders.
packages/extension-polkagate/src/fullscreen/homeFullScreen/components/ChainList.tsx (2)
9-10: LGTM! Import statements are properly optimized.
The imports have been streamlined by removing unused components and adding only the necessary icons.
Line range hint 1-178: Verify integration with related components.
The changes to ChainList are part of a larger refactor involving CurrencyList, Currency, and FavoriteChains components. Let's verify the consistency of the changes across these components.
✅ Verification successful
Let me gather more context about the related components to verify the integration.
Based on the gathered information, I can now provide a final response:
Integration is consistent across components
The changes in ChainList are well-integrated with related components:
CurrencyListfollows the same UI pattern withInputFilterand scrollable Grid containerDEFAULT_SELECTED_CHAINSis properly used in bothChainListanduseSelectedChainshookshowOtherChainsreferences inChainSwitchare independent and unrelated to this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent implementation across related components
# Check for similar patterns in related components
echo "Checking for consistent implementation patterns..."
rg -l "InputFilter|Grid container.*overflow.*scroll" packages/extension-polkagate/src/fullscreen/homeFullScreen/components/
# Check for any remaining references to removed functionality
echo "Checking for any remaining references to removed functionality..."
rg "showOtherChains|DEFAULT_SELECTED_CHAINS_COUNT" packages/extension-polkagate/src/
Length of output: 1728
Script:
#!/bin/bash
# Check the implementation of CurrencyList and ChainSwitch
echo "=== CurrencyList implementation ==="
cat packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyList.tsx
echo -e "\n=== ChainSwitch implementation ==="
cat packages/extension-polkagate/src/partials/ChainSwitch.tsx
# Check for any dependencies on DEFAULT_SELECTED_CHAINS
echo -e "\n=== Usage of DEFAULT_SELECTED_CHAINS ==="
rg -A 2 "DEFAULT_SELECTED_CHAINS" packages/extension-polkagate/src/
Length of output: 17177
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/Currency.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/FavoriteChains.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/FavoriteChains.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyList.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (13)
packages/extension-polkagate/src/components/InputFilter.tsx (2)
25-25: Consider extracting default values to constantsWhile the implementation is correct, consider extracting the default values (especially '18px') to named constants at the file level. This would improve maintainability and make it easier to update default values consistently.
+const DEFAULT_FONT_SIZE = '18px'; +const DEFAULT_AUTO_FOCUS = true; +const DEFAULT_WITH_RESET = false; -export default function InputFilter ({ autoFocus = true, disabled, fontSize = '18px', label, onChange, placeholder, theme, value, withReset = false }: Props) { +export default function InputFilter ({ autoFocus = DEFAULT_AUTO_FOCUS, disabled, fontSize = DEFAULT_FONT_SIZE, label, onChange, placeholder, theme, value, withReset = DEFAULT_WITH_RESET }: Props) {
Line range hint
52-56: Consider extracting styles for better reusabilityThe inline styles could be extracted into a separate constant or styled-component for better reusability and maintainability. This would also make it easier to maintain consistent styling across different instances of the component.
+const getInputStyles = (fontSize: string) => ({ + fontSize, + fontWeight: 300, + padding: 0, + paddingLeft: '10px' +}); <Input // ... other props ... - style={{ - fontSize, - fontWeight: 300, - padding: 0, - paddingLeft: '10px' - }} + style={getInputStyles(fontSize)} // ... other props ... />packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyItem.tsx (1)
48-62: Consider accessibility and maintainability improvementsThe new layout structure using Stack and Grid is well-organized and provides better control over spacing. However, there are some potential improvements:
- Consider adding focus states for keyboard navigation:
- sx={{ ':hover': { bgcolor: theme.palette.mode === 'light' ? 'rgba(24, 7, 16, 0.1)' : 'rgba(255, 255, 255, 0.1)' }, cursor: 'pointer', height: '45px', px: '15px' }} + sx={{ + ':hover, :focus-visible': { + bgcolor: theme.palette.mode === 'light' ? 'rgba(24, 7, 16, 0.1)' : 'rgba(255, 255, 255, 0.1)', + outline: 'none' + }, + cursor: 'pointer', + height: '45px', + px: '15px' + }}
- Consider extracting magic numbers to theme constants for better maintainability:
const CURRENCY_ITEM_HEIGHT = '45px'; const CURRENCY_ITEM_PADDING = '15px'; const FLAG_ICON_SIZE = '17px';packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/FavoriteChains.tsx (1)
16-16: Consider movingCurrencyItemTypeinterface to a more appropriate location.The
CurrencyItemTypeinterface appears to be currency-related but is defined in a chains-focused component. Consider moving it to a shared types file or to a currency-related component for better code organization.packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyList.tsx (3)
23-28: Consider enhancing prop types for better type safety.The component structure looks good, but we can improve type safety:
interface Props { - anchorEl: HTMLButtonElement | null; - setAnchorEl: React.Dispatch<React.SetStateAction<HTMLButtonElement | null>>; + anchorEl: HTMLButtonElement | null; + setAnchorEl: (el: HTMLButtonElement | null) => void; + // Add JSDoc comments for better documentation + /** Callback to update the displayed currency */ setCurrencyToShow: React.Dispatch<React.SetStateAction<CurrencyItemType | undefined>>; }
Line range hint
35-61: Optimize event handlers for better performance and error handling.A few suggestions for improvement:
const changeCurrency = useCallback((currency: CurrencyItemType) => { setAnchorEl(null); setCurrency(currency); setCurrencyToShow(currency); - setStorage('currency', currency).catch(console.error); + setStorage('currency', currency).catch((error) => { + console.error('Failed to save currency preference:', error); + // Consider showing a user-friendly error message + }); }, [setAnchorEl, setCurrency, setCurrencyToShow]); const onSearch = useCallback((keyword: string) => { if (!keyword) { setSearchKeyword(''); return setSearchedCurrency(undefined); } setSearchKeyword(keyword); - keyword = keyword.trim().toLowerCase(); + const normalizedKeyword = keyword.trim().toLowerCase(); - const filtered = CURRENCY_LIST.filter((currency) => + // Avoid filtering if the search term is too short + if (normalizedKeyword.length < 2) { + return setSearchedCurrency(undefined); + } + + const filtered = CURRENCY_LIST.filter((currency) => ( currency.code.toLowerCase().includes(keyword) || currency.country.toLowerCase().includes(keyword) || currency.currency.toLowerCase().includes(keyword) - ); + )); - setSearchedCurrency([...filtered]); + setSearchedCurrency(filtered); }, []);
64-70: Consider adjusting transition duration for better UX.The transition duration of 5000ms (5 seconds) seems excessive and might lead to a sluggish user experience.
- <Grid container item sx={{ transition: 'height 5000ms ease-in-out', width: '230px' }}> + <Grid container item sx={{ transition: 'height 300ms ease-in-out', width: '230px' }}>packages/extension-polkagate/src/util/currencyList.tsx (3)
13-31: Consider renaming the "country" field for cryptocurrenciesThe "country" field is being used to store blockchain/network names for cryptocurrencies, which might be confusing. Consider renaming this field to something more generic like "network" or "system" to better represent both traditional currencies and cryptocurrencies.
const CRYPTO_AS_CURRENCY = [{ code: 'BTC', - country: 'Bitcoin', + network: 'Bitcoin', currency: 'Bitcoin', sign: '₿' }, { code: 'ETH', - country: 'Ethereum', + network: 'Ethereum', currency: 'Ethereum', sign: 'Ξ' }, { code: 'DOT', - country: 'Polkadot', + network: 'Polkadot', currency: 'Polkadot', sign: '𝒫' } ];
Line range hint
89-100: Fix duplicate CNY entry with incorrect Czech Koruna informationThere's a duplicate CNY entry that appears to be incorrectly representing the Czech Koruna (CZK). This needs to be fixed as it could cause currency conversion issues.
- { - code: 'CNY', - country: 'Czech', - currency: 'Koruna', - sign: 'CZK' - }, + { + code: 'CZK', + country: 'Czech Republic', + currency: 'Koruna', + sign: 'Kč' + },
Line range hint
1-5: Consider consolidating cryptocurrency references
ASSETS_AS_CURRENCY_LISTandCRYPTO_AS_CURRENCYcontain overlapping information about the same cryptocurrencies. Consider deriving one from the other to avoid potential maintenance issues.-export const ASSETS_AS_CURRENCY_LIST = ['BTC', 'ETH', 'DOT']; +export const ASSETS_AS_CURRENCY_LIST = CRYPTO_AS_CURRENCY.map(crypto => crypto.code);packages/extension-polkagate/src/fullscreen/homeFullScreen/components/ChainList.tsx (3)
Line range hint
134-156: Consider adding confirmation for reset actionThe reset functionality directly resets to default chains without user confirmation. This could lead to accidental data loss if clicked unintentionally.
Consider adding a confirmation dialog before resetting, especially since this affects saved assets. Example implementation:
const onReset = useCallback(() => { + const confirmReset = window.confirm(t('Are you sure you want to reset to default chains? This will affect your saved assets.')); + if (!confirmReset) return; const defaultSelectedGenesisHashes = DEFAULT_SELECTED_CHAINS.map(({ value }) => value as string); setInitialChains(new Set(defaultSelectedGenesisHashes)); setStorage('selectedChains', defaultSelectedGenesisHashes).catch(console.error); updateSavedAssetsInStorage(); }, [updateSavedAssetsInStorage]);
157-164: Reconsider autoFocus on search inputWhile autoFocus can be convenient, it might interfere with the user's workflow, especially if they want to browse the list first before searching.
Consider removing autoFocus unless there's a specific UX requirement for it:
<InputFilter - autoFocus fontSize='14px' onChange={onSearch} placeholder={t('🔍 Search chain')} theme={theme} value={searchKeyword ?? ''} />
Line range hint
167-177: Performance and styling improvements neededSeveral issues need attention in the chain list container:
- The transition duration of 5000ms (5 seconds) seems excessive
- Fixed maxHeight calculation might not be responsive
- Consider virtualization for better performance with large lists
Consider these improvements:
<Grid container item sx={{ - maxHeight: 'calc(100vh - 200px)', + maxHeight: 'min(600px, calc(100vh - 200px))', overflow: 'hidden', overflowY: 'scroll', - transition: 'height 5000ms ease-in-out' + transition: 'height 300ms ease-in-out' }}>For better performance with large lists, consider using a virtualization library like
react-window:import { FixedSizeList } from 'react-window'; const Row = ({ index, style }) => { const item = (searchedChain ?? sortedChainsToShow)[index]; return ( <div style={style}> <ChainItem chain={item} disabled={!isTestnetEnabled && TEST_NETS.includes(item.value as string)} isSelected={selectedChains.has(item.value as string)} onclick={onChainSelect} /> </div> ); }; // In render: <FixedSizeList height={Math.min(600, window.innerHeight - 200)} itemCount={(searchedChain ?? sortedChainsToShow).length} itemSize={50} width="100%" > {Row} </FixedSizeList>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
packages/extension-polkagate/src/components/InputFilter.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/components/ChainList.tsx(4 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyItem.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyList.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/Currency.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/FavoriteChains.tsx(3 hunks)packages/extension-polkagate/src/util/currencyList.tsx(1 hunks)packages/extension/public/locales/en/translation.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/Currency.tsx
🔇 Additional comments (12)
packages/extension-polkagate/src/components/InputFilter.tsx (1)
15-16: LGTM: Props interface changes are well-structured
The addition of disabled and fontSize as optional props enhances the component's configurability while maintaining type safety.
packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyItem.tsx (1)
7-7: LGTM: Import changes are appropriate
The addition of Stack from @mui/material is properly organized with other MUI imports and supports the new layout implementation.
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/FavoriteChains.tsx (4)
79-81: Improve dialog positioning robustness.
The current positioning logic using getBoundingClientRect with hardcoded offsets might break on window resize or scroll.
Consider using MUI's built-in positioning system:
- left: anchorEl?.getBoundingClientRect().right - 310,
- position: 'absolute',
- top: anchorEl?.getBoundingClientRect().bottom - 30
+ position: 'fixed',
+ top: '50%',
+ left: '50%',
+ transform: 'translate(-50%, -50%)'54-63: Improve accessibility by using semantic HTML.
The Grid component is still being used as a button.
Consider using MUI's Button component for better accessibility:
- <Grid
- alignItems='center'
- component='button'
- container
- direction='column'
- item
- justifyContent='center'
- onClick={onChainListClick}
- sx={{ ...HEADER_COMPONENT_STYLE, zIndex: anchorEl && theme.zIndex.modal + 1 }}
- >
+ <Button
+ onClick={onChainListClick}
+ sx={{
+ ...HEADER_COMPONENT_STYLE,
+ display: 'flex',
+ flexDirection: 'column',
+ alignItems: 'center',
+ justifyContent: 'center',
+ zIndex: anchorEl && theme.zIndex.modal + 1
+ }}
+ >62-62: Good handling of zIndex for modal stacking.
The dynamic zIndex adjustment when the dialog is open ensures proper stacking order with other modal elements.
73-93: Well-implemented styling with proper theme integration.
The Dialog styling shows good attention to detail:
- Proper theme-aware box shadows
- Nice backdrop blur effect
- Consistent border radius
- Good use of theme variables
packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyList.tsx (3)
29-33: LGTM! Clean state management implementation.
The state management and cleanup logic is well-implemented, with proper effect dependencies and state reset on dialog close.
94-94: LGTM! Appropriate use of memoization.
The use of React.memo is justified here as the component receives props that don't change frequently.
81-89: 🛠️ Refactor suggestion
Improve accessibility and scroll behavior.
The previous review comment about accessibility is still valid. Additionally, consider using auto instead of scroll for better UX.
- <Grid container item sx={{ maxHeight: 'calc(100vh - 200px)', overflow: 'hidden', overflowY: 'scroll' }}>
- {[...(searchedCurrency ?? CURRENCY_LIST)].map((item, index) => (
+ <Grid
+ container
+ item
+ role="listbox"
+ aria-label={t('Available currencies')}
+ sx={{ maxHeight: 'calc(100vh - 200px)', overflow: 'hidden', overflowY: 'auto' }}
+ >
+ {(searchedCurrency ?? CURRENCY_LIST).map((item, index) => (
<CurrencyItem
currency={item}
key={index}
onclick={changeCurrency}
+ role="option"
+ tabIndex={0}
/>
))}packages/extension-polkagate/src/util/currencyList.tsx (1)
34-34: LGTM! Clean integration of cryptocurrency entries
The spread operator usage is appropriate, and placing cryptocurrencies at the start of the list provides good visibility.
packages/extension-polkagate/src/fullscreen/homeFullScreen/components/ChainList.tsx (1)
Line range hint 67-77: Verify impact on saved assets
The code removes assets data for unselected chains. We should verify that this doesn't cause issues with chain re-selection or data restoration.
Let's verify the assets storage structure and usage:
✅ Verification successful
Let me analyze the code further by checking the assets storage structure and restoration logic.
Let me try a different approach to verify the assets storage handling.
Based on the gathered information, I can now provide the final response:
Chain selection and assets storage handling is properly implemented
The code correctly handles chain selection and asset storage:
- Assets are properly cleaned up when chains are unselected via
updateSavedAssetsInStorage - Chain re-selection is handled safely through
handleChainsChanges - The assets storage has proper initialization, watching, and restoration logic in
useAssetsBalances - Changes to selected chains trigger appropriate storage updates for both chain selection and related assets
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues with assets storage handling
# Check for other places where ASSETS_NAME_IN_STORAGE is accessed
rg "ASSETS_NAME_IN_STORAGE" -A 5
# Look for related storage operations
rg "setStorage\(.*ASSETS_NAME_IN_STORAGE" -A 5
Length of output: 8674
Script:
#!/bin/bash
# Check the assets storage initialization and restoration logic
ast-grep --pattern 'const useAssetsBalances = () => {
$$$
}'
# Check for chain selection restoration logic
rg "selectedChains.*getStorage" -A 5
Length of output: 103
Script:
#!/bin/bash
# Check the initialization and restoration of selected chains
rg "const.*selectedChains.*=" -A 5
# Check the hook implementation for assets balances
rg "export.*useAssetsBalances" -B 5 -A 15
# Look for any chain reselection handling
rg "handleChainsChanges" -B 5 -A 5
Length of output: 12420
packages/extension/public/locales/en/translation.json (1)
1464-1465: LGTM! Translation entries are clear and consistent.
The changes look good:
- Added trailing comma to "Track Id" entry maintains JSON formatting consistency
- New "Select the currency for your balance" entry provides a clear prompt for the currency selection UI
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyList.tsx (2)
Line range hint
42-57: Optimize search functionality performance.Consider debouncing the search operation to prevent excessive filtering on every keystroke, especially with larger currency lists.
+import { debounce } from 'lodash'; -const onSearch = useCallback((keyword: string) => { +const onSearch = useCallback( + debounce((keyword: string) => { if (!keyword) { setSearchKeyword(''); return setSearchedCurrency(undefined); } setSearchKeyword(keyword); keyword = keyword.trim().toLowerCase(); const filtered = CURRENCY_LIST.filter((currency) => currency.code.toLowerCase().includes(keyword) || currency.country.toLowerCase().includes(keyword) || currency.currency.toLowerCase().includes(keyword) ); setSearchedCurrency([...filtered]); - }, []); + }, 300), + [] +);
71-89: Further enhance accessibility and UX.While basic accessibility roles are implemented, there's room for improvement in keyboard navigation and ARIA attributes.
-<Grid container p='5px' sx={{ display: 'list-item' }}> +<Grid container p='5px' sx={{ display: 'list-item' }} role="search"> <InputFilter autoFocus fontSize='14px' onChange={onSearch} placeholder={t('🔍 Search currency')} theme={theme} value={searchKeyword ?? ''} + aria-label={t('Search currencies')} /> </Grid> -<Grid container item role='listbox' sx={{ maxHeight: 'calc(100vh - 200px)', overflow: 'hidden', overflowY: 'scroll' }}> +<Grid + container + item + role='listbox' + aria-label={t('Currency list')} + sx={{ + maxHeight: 'calc(100vh - 200px)', + overflow: 'hidden', + overflowY: 'auto', + '&:focus': { + outline: `2px solid ${theme.palette.primary.main}`, + outlineOffset: '2px' + } + }} +> {(searchedCurrency ?? CURRENCY_LIST).map((item, index) => ( <CurrencyItem currency={item} key={index} onclick={changeCurrency} + role="option" + aria-selected={false} + tabIndex={0} /> ))} </Grid>packages/extension-polkagate/src/fullscreen/homeFullScreen/components/ChainList.tsx (2)
Line range hint
134-156: Consider improving layout responsiveness and reducing style duplicationWhile the layout structure is well-organized, there are opportunities for improvement:
- The fixed width of 280px might not be optimal for all screen sizes
- The button styles contain duplicated hover and icon margin properties
Consider these improvements:
-<Grid container item sx={{ width: '280px' }}> +<Grid container item sx={{ width: { xs: '100%', sm: '280px' } }}> // Extract common button styles to reduce duplication +const commonButtonStyle = { + '&:hover': { bgcolor: 'divider' }, + '.MuiButton-startIcon': { marginRight: '2px' }, + color: theme.palette.secondary.light, + fontSize: '12px', + fontWeight: 300, + p: 0, + textTransform: 'none', + width: 'fit-content' +}; -sx={{ '&:hover': { bgcolor: 'divider' }, '.MuiButton-startIcon': { marginRight: '2px' }, color: theme.palette.secondary.light, fontSize: '12px', fontWeight: 300, ml: '10px', p: 0, textTransform: 'none', width: 'fit-content' }} +sx={{ ...commonButtonStyle, ml: '10px' }}
Line range hint
167-177: Address performance and React best practices concernsThe chain list rendering has several issues that should be addressed:
- The transition duration of 5000ms (5 seconds) is extremely long and might affect user experience
- Using array index as key prop might cause issues with React's reconciliation when items are reordered
Consider these improvements:
-<Grid container item sx={{ maxHeight: 'calc(100vh - 200px)', overflow: 'hidden', overflowY: 'scroll', transition: 'height 5000ms ease-in-out' }}> +<Grid container item sx={{ maxHeight: 'calc(100vh - 200px)', overflow: 'hidden', overflowY: 'auto', transition: 'height 300ms ease-in-out' }}> {(searchedChain ?? sortedChainsToShow).map((item, index) => ( <ChainItem chain={item} disabled={!isTestnetEnabled && TEST_NETS.includes(item.value as string)} isSelected={selectedChains.has(item.value as string)} - key={index} + key={item.value} onclick={onChainSelect} /> ))}Also, consider using
overflowY: 'auto'instead of 'scroll' to avoid showing unnecessary scrollbars.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/extension-polkagate/src/fullscreen/homeFullScreen/components/ChainList.tsx(4 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyList.tsx(3 hunks)
🔇 Additional comments (2)
packages/extension-polkagate/src/fullscreen/homeFullScreen/components/ChainList.tsx (2)
9-10: LGTM! Import changes align with new UI requirements
The updated imports properly reflect the component's new functionality, removing unused components and adding necessary icons for the new features.
157-164: Review search UX considerations
A few points about the search implementation:
- The
autoFocusprop might interrupt users who are using keyboard navigation - The emoji in the placeholder text might not render consistently across all platforms
Let's check the InputFilter component's implementation:
| transform: showOtherCurrencies ? 'rotate(-90deg)' : 'rotate(90deg)', | ||
| transition: 'transform 150ms ease-in-out' | ||
| }} | ||
| <Grid container item sx={{ transition: 'height 5000ms ease-in-out', width: '230px' }}> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust transition duration for better UX.
The current transition duration of 5000ms (5 seconds) seems excessive and might make the UI feel sluggish.
-<Grid container item sx={{ transition: 'height 5000ms ease-in-out', width: '230px' }}>
+<Grid container item sx={{ transition: 'height 300ms ease-in-out', width: '230px' }}>📝 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.
| <Grid container item sx={{ transition: 'height 5000ms ease-in-out', width: '230px' }}> | |
| <Grid container item sx={{ transition: 'height 300ms ease-in-out', width: '230px' }}> |
Summary by CodeRabbit
Release Notes
New Features
Improvements
InputFiltercomponent with customizable font size.Bug Fixes
These changes streamline the user experience and improve the overall functionality of the application.