feat: add filter to NFT page #1639
Conversation
WalkthroughThis pull request introduces several modifications across multiple components in the Polkadot extension. Notably, a new Changes
Possibly related PRs
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: 5
🧹 Outside diff range and nitpick comments (9)
packages/extension-polkagate/src/fullscreen/nft/utils/types.ts (1)
118-128: Consider enhancing filter type definitions for better maintainability.The filter interfaces could benefit from the following improvements:
- Add JSDoc comments explaining the purpose of each filter
- Consider using an enum or union type for filter values instead of booleans to support future multi-select options
- Consider renaming ambiguous filters for clarity (e.g., 'unique', 'nft')
Here's a suggested improvement:
+/** Represents the state of NFT filters */ export interface FilterState { + /** Filter by NFT collections */ collections: boolean; + /** Filter by individual NFTs */ nft: boolean; - unique: boolean; + /** Filter by RMRK standard NFTs */ + rmrk: boolean; /** Filter by Kusama network */ kusama: boolean; /** Filter by Polkadot network */ polkadot: boolean; } +/** Action to update a specific filter */ export interface FilterAction { filter: keyof FilterState; } +/** Initial state for filters */ +export const initialFilterState: FilterState = { + collections: false, + nft: false, + rmrk: false, + kusama: false, + polkadot: false +};packages/extension-polkagate/src/fullscreen/nft/index.tsx (2)
61-61: Consider memoization optimization.While adding nftManager to the dependency array is correct, consider wrapping the handleNftUpdate callback with useCallback to prevent unnecessary re-renders if the nftManager reference changes.
+ const handleNftUpdate = useCallback((updatedAddress: string, updatedNfts: ItemInformation[]) => { + if (updatedAddress === address) { + setNfts(updatedNfts); + } + }, [address]); useEffect(() => { const myNfts = nftManager.get(address); setNfts(myNfts); - const handleNftUpdate = (updatedAddress: string, updatedNfts: ItemInformation[]) => { - if (updatedAddress === address) { - setNfts(updatedNfts); - } - }; nftManager.subscribe(handleNftUpdate); return () => { nftManager.unsubscribe(handleNftUpdate); }; }, [address, nftManager, handleNftUpdate]);
Line range hint
127-131: Add loading and error states for filtered items.While the Filters integration looks good, consider handling loading and empty states for itemsToShow to improve user experience.
<Filters items={nfts} setItemsToShow={setItemsToShow} /> + {itemsToShow === undefined && ( + <Typography>Loading NFTs...</Typography> + )} + {itemsToShow === null && ( + <Typography>No NFTs found</Typography> + )} <NftList apis={apis} nfts={itemsToShow} />packages/extension-polkagate/src/fullscreen/nft/components/NftFilter.tsx (3)
4-14: Consider organizing imports for better maintainability.Group the imports into these categories for better organization:
- External libraries (React, MUI)
- Internal components/hooks
- Types/interfaces
/* eslint-disable react/jsx-max-props-per-line */ import type { FilterAction, FilterState, SortAction, SortState } from '../utils/types'; + // External imports import React, { useCallback } from 'react'; import { FilterAltOutlined as FilterIcon, FilterList as FilterListIcon, ImportExport as ImportExportIcon } from '@mui/icons-material'; import { Divider, FormControl, FormControlLabel, Grid, Popover, Radio, RadioGroup, Typography, useTheme } from '@mui/material'; + // Internal imports import Checkbox2 from '../../../components/Checkbox2'; import { useTranslation } from '../../../hooks';
15-20: Add JSDoc documentation for the Props interface.Adding documentation will improve code maintainability and help other developers understand the purpose of each prop.
+/** + * Props for the Filters and NftFilters components + * @property {React.Dispatch<FilterAction>} dispatchFilter - Dispatch function for filter actions + * @property {FilterState} filters - Current filter state + * @property {React.Dispatch<SortAction>} dispatchSort - Dispatch function for sort actions + * @property {SortState} sort - Current sort state + */ interface Props { dispatchFilter: React.Dispatch<FilterAction>; filters: FilterState; dispatchSort: React.Dispatch<SortAction>; sort: SortState; }
30-32: Fix parameter naming in onSort callback.The parameter name 'unable' appears to be a typo and should be 'disable' to better reflect its purpose.
-const onSort = useCallback((enable: keyof SortState, unable: keyof SortState) => () => { - dispatchSort({ enable, unable }); +const onSort = useCallback((enable: keyof SortState, disable: keyof SortState) => () => { + dispatchSort({ enable, disable }); }, [dispatchSort]);packages/extension-polkagate/src/fullscreen/nft/components/Details.tsx (3)
287-288: Consider simplifying the divider logic.The divider condition
!!itemInformation.description || (!itemInformation.isCollection && !!itemInformation.collectionName)is complex and may be difficult to maintain. Consider extracting this logic into a separate function or adopting a more consistent approach to dividers.+ const shouldShowDivider = useCallback(() => { + return !!itemInformation.description || + (!itemInformation.isCollection && !!itemInformation.collectionName); + }, [itemInformation.description, itemInformation.isCollection, itemInformation.collectionName]); <InfoRow - divider={!!itemInformation.description || (!itemInformation.isCollection && !!itemInformation.collectionName)} + divider={shouldShowDivider()} text={itemInformation.chainName} title={t('Network')} />Alternatively, consider using a more consistent pattern for dividers, such as showing them between all rows except the last one, which would be more maintainable and predictable.
Line range hint
199-203: Improve fullscreen error handling.The current error handling for fullscreen requests only logs errors to the console. Consider handling these errors more gracefully by showing a user-friendly message when fullscreen requests fail.
const openFullscreen = useCallback(() => { if (showFullscreenDisabled) { return; } - document.documentElement.requestFullscreen().catch(console.error); + document.documentElement.requestFullscreen().catch((error) => { + console.error('Fullscreen request failed:', error); + // Show user-friendly error message + // Consider adding a toast notification or inline message + }); setShowFullscreen(true); }, [showFullscreenDisabled]);
Line range hint
1-289: Consider breaking down the component for better maintainability.The Details component is quite large and handles multiple responsibilities (media display, fullscreen management, information display). Consider splitting it into smaller, focused components:
- MediaDisplay - Handle different media types (images, GIFs, audio, HTML)
- InfoSection - Handle the information display and divider logic
- FullscreenControls - Manage fullscreen functionality
This would improve:
- Code organization and maintainability
- Testing capabilities
- Reusability of individual components
- State management clarity
Would you like me to provide a detailed example of this restructuring?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/extension-polkagate/src/fullscreen/nft/components/Details.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/nft/components/Filters.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/nft/components/NftFilter.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/nft/components/Tabs.tsx(0 hunks)packages/extension-polkagate/src/fullscreen/nft/index.tsx(4 hunks)packages/extension-polkagate/src/fullscreen/nft/utils/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/extension-polkagate/src/fullscreen/nft/components/Tabs.tsx
🔇 Additional comments (5)
packages/extension-polkagate/src/fullscreen/nft/index.tsx (2)
21-21: LGTM: Clean state management implementation.
The new imports and state variables are well-typed and properly integrated. The use of an enum for step management provides good type safety.
Also applies to: 41-42
21-21: Verify removal of Tabs component.
Let's ensure no other components depend on the removed Tabs component.
packages/extension-polkagate/src/fullscreen/nft/components/NftFilter.tsx (2)
153-153: LGTM!
The component is properly memoized before export to prevent unnecessary re-renders.
6-6: Verify type definitions and component integration.
Let's verify the type definitions and component usage in the codebase.
✅ Verification successful
Types are properly integrated and used correctly
The imported types FilterAction, FilterState, SortAction, and SortState are correctly defined in utils/types.ts and properly used in both NftFilter and Filters components. The integration shows:
- Types are properly defined in the types file
- Correct usage in reducer functions and component props
- Proper integration with parent component (Filters)
- Consistent type usage across the filter functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify type definitions and component integration
# Check type definitions
echo "Checking type definitions..."
ast-grep --pattern 'type FilterAction = { $$$; };'
ast-grep --pattern 'type SortAction = { $$$; };'
# Check component usage
echo "Checking component usage..."
rg -l "import.*NftFilters?" --type tsx
Length of output: 304
Script:
#!/bin/bash
# Let's try a different approach to verify the types and component usage
# Check for type definitions in utils/types
echo "Checking type definitions in utils/types..."
fd "types.ts" --exec cat {}
# Check component usage with different extension
echo "Checking component usage..."
rg -l "import.*NftFilter" --type ts --type tsx
# Look for FilterAction and SortAction usage
echo "Checking type usage..."
rg "FilterAction|SortAction" --type ts --type tsx
Length of output: 48344
Script:
#!/bin/bash
# Let's try without specifying file types for rg
# Check for type definitions
echo "Checking type definitions..."
rg -p "type (FilterAction|FilterState|SortAction|SortState)"
# Check component usage
echo "Checking component usage..."
rg -p "import.*NftFilter"
# Look for usage of these types
echo "Checking type usage..."
rg -p "(FilterAction|SortAction) *[=:]"
Length of output: 430
Script:
#!/bin/bash
# Let's check the content of related files
# Check the content of NftFilter component
echo "Checking NftFilter component..."
cat packages/extension-polkagate/src/fullscreen/nft/components/NftFilter.tsx
# Check the content of Filters component
echo "Checking Filters component..."
cat packages/extension-polkagate/src/fullscreen/nft/components/Filters.tsx
# Check the types file
echo "Checking types file..."
cat packages/extension-polkagate/src/fullscreen/nft/utils/types.ts
Length of output: 15111
packages/extension-polkagate/src/fullscreen/nft/components/Filters.tsx (1)
63-65:
Ensure chainName is defined before using toLowerCase()
In the getDecimal function, chainName might be undefined, which would cause an error when calling toLowerCase().
Consider adding a check for chainName:
const getDecimal = useCallback((chainName: string) => {
+ if (!chainName) return undefined;
return selectableNetworks.find(({ network }) => network.toLowerCase() === chainName.toLowerCase())?.decimals[0];
}, []);Likely invalid or redundant comment.
packages/extension-polkagate/src/fullscreen/nft/components/NftFilter.tsx
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/nft/components/NftFilter.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/extension-polkagate/src/fullscreen/nft/components/InfoRow.tsx (1)
47-47: Consider centralizing typography styles for better maintainability.The font weight changes from 400 to 500 across multiple typography elements suggest an effort to establish a consistent visual hierarchy. While the changes themselves are valid, having these styles scattered across individual components could lead to maintenance challenges.
Consider:
- Creating a theme-based typography system
- Using styled components or a CSS-in-JS solution
- Defining typography variants in a central location
Example approach using MUI theme customization:
// theme.ts const theme = createTheme({ typography: { nftLabel: { fontSize: '14px', fontWeight: 500, }, // other variants... }, }); // Usage in component <Typography variant="nftLabel"> {t('Not listed')} </Typography>Also applies to: 69-69, 74-74
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/extension-polkagate/src/fullscreen/nft/components/InfoRow.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/nft/components/Thumbnail.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension-polkagate/src/fullscreen/nft/components/Thumbnail.tsx
🔇 Additional comments (1)
packages/extension-polkagate/src/fullscreen/nft/components/InfoRow.tsx (1)
47-47: Verify accessibility compliance with font weight changes.
The increased font weight (500) improves visual prominence but should be tested for accessibility, particularly for color contrast ratios and readability.
Also applies to: 69-69, 74-74
✅ Verification successful
Based on the comprehensive search results showing Typography component usage across the codebase, I can now generate the final response:
Font weight 500 is consistently used for emphasis and visual hierarchy throughout the codebase
The font weight of 500 in the InfoRow component aligns with the established pattern across the codebase where this weight is commonly used for:
- Section headers and labels
- Important information that needs visual emphasis
- Interactive elements like menu items and buttons
- Selected/active states
The usage maintains accessibility standards as:
- It provides sufficient contrast with regular text (400) and light text (300)
- It's used sparingly and purposefully for visual hierarchy
- It follows Material UI's recommended font weight scale
- It's consistently applied across similar UI elements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other typography elements in the codebase to ensure consistent font weight usage
rg -l "Typography.*fontWeight" | while read -r file; do
echo "=== $file ==="
rg "Typography.*fontWeight" "$file"
done
Length of output: 163280
# [0.25.0](v0.24.0...v0.25.0) (2024-11-10) ### Features * add filter to NFT page ([#1639](#1639)) ([c8c835e](c8c835e))
close: #1638
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Style