feat: add account icon theme options #1612
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several changes across multiple components in the Changes
Suggested labels
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: 5
🧹 Outside diff range and nitpick comments (5)
packages/extension-polkagate/src/components/SelectIdenticonTheme.tsx (1)
34-38: Simplify Grid's sx prop spreading.The current style spreading is unnecessarily verbose for a single prop.
<Grid container item - sx={{ ...style }} + sx={style} >packages/extension-polkagate/src/components/Select2.tsx (2)
26-32: Consider adding JSDoc comments for the new props.While the prop names are self-descriptive, adding JSDoc comments would improve maintainability by documenting:
- Expected values for fontSize and padding props
- The purpose of isIdenticon and its relationship with showIcons
- The visual impact of the rounded prop
Example documentation:
interface Props { /** Font size for the label. Default: '10px' */ labelFontSize?: string; /** Left padding for the label. Default: '5px' */ labelPaddingLeft?: string; /** Whether to show polkadot identicon instead of chain logo */ isIdenticon?: boolean; /** Whether to use rounded corners (20px) or slight rounded corners (5px) */ rounded?: boolean; // ... other props }
165-167: Consider improving accessibility for overflowing text.The layout changes look good, but consider adding a title attribute to help users see the full text when it overflows:
<Grid alignItems='center' container item justifyContent='flex-start' pl='6px' sx={{ overflowX: 'scroll' }} width='fit-content'> - <Typography fontSize={textFontSize} fontWeight={300}> + <Typography fontSize={textFontSize} fontWeight={300} title={text}> {text} </Typography> </Grid>Also applies to: 171-171, 213-214
packages/extension-polkagate/src/util/constants.tsx (1)
247-247: Add a descriptive comment for the demo account.While moving the demo account address to constants improves maintainability, please add a comment explaining its purpose and usage.
+// Demo account address used for onboarding and testing purposes export const DEMO_ACCOUNT = '1ChFWeNRLarAPRCTM3bfJmncJbSAbSS9yqjueWz7jX7iTVZ';packages/extension-ui/src/Popup/index.tsx (1)
142-142: Consider enhancing the theme persistence logic.While the implementation is functional, consider these improvements:
- Add setAccountIconTheme to the effect's dependency array to handle potential setter changes
- Implement more specific error handling instead of using console.error
useEffect(() => { getStorage('iconTheme') .then((maybeTheme) => setAccountIconTheme((maybeTheme as IconTheme | undefined) || DEFAULT_ACCOUNT_ICON_THEME)) - .catch(console.error); + .catch((error: Error) => { + console.error('Failed to load icon theme:', error.message); + setAccountIconTheme(DEFAULT_ACCOUNT_ICON_THEME); + }); - }, []); + }, [setAccountIconTheme]);Also applies to: 162-166
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- packages/extension-polkagate/src/components/Identicon.tsx (3 hunks)
- packages/extension-polkagate/src/components/Select2.tsx (7 hunks)
- packages/extension-polkagate/src/components/SelectIdenticonTheme.tsx (1 hunks)
- packages/extension-polkagate/src/components/contexts.tsx (2 hunks)
- packages/extension-polkagate/src/components/index.ts (1 hunks)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/SettingSubMenuFullScreen.tsx (5 hunks)
- packages/extension-polkagate/src/fullscreen/onboarding/index.tsx (2 hunks)
- packages/extension-polkagate/src/partials/SettingSubMenu.tsx (3 hunks)
- packages/extension-polkagate/src/util/constants.tsx (1 hunks)
- packages/extension-polkagate/src/util/types.ts (2 hunks)
- packages/extension-ui/src/Popup/index.tsx (6 hunks)
- packages/extension/public/locales/en/translation.json (1 hunks)
🔇 Additional comments (27)
packages/extension-polkagate/src/components/SelectIdenticonTheme.tsx (3)
1-13: LGTM! Imports are well-organized and properly typed.The imports are logically grouped (external libraries followed by local imports) and include all necessary dependencies.
55-55: LGTM! Proper memoization and export.The component is correctly memoized to prevent unnecessary re-renders, and the export follows React best practices.
19-53: Verify theme persistence and context integration.Let's ensure the theme selection is properly persisted and the context is correctly integrated across the application.
✅ Verification successful
Let me gather more information about the theme persistence implementation since the previous search for
.tsxfiles didn't work properly.
Let me try a different approach using just the file extension without the type flag:
Based on the gathered information, I can now provide the final response:
Theme persistence and context integration are properly implemented ✓
The implementation shows a well-integrated theme system with:
- Context properly defined in
packages/extension-polkagate/src/components/contexts.tsx- Theme persistence using
setStorage('iconTheme', ...)consistently- Context consumed correctly in multiple components:
SelectIdenticonTheme.tsxfor theme selectionIdenticon.tsxfor theme application- Provider setup in
packages/extension-ui/src/Popup/index.tsx🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify theme persistence implementation and context usage # Check for proper context provider setup echo "Checking for AccountIconThemeContext provider setup..." rg -l "AccountIconThemeContext.Provider" # Check for consistent theme storage key usage echo "Checking for consistent theme storage key usage..." rg "iconTheme" --type ts --type tsx # Check for other components using the theme context echo "Checking for components consuming the theme context..." ast-grep --pattern 'useContext(AccountIconThemeContext)'Length of output: 844
Script:
#!/bin/bash # Check theme storage implementation and context definition echo "Checking theme storage and context implementation..." rg "iconTheme" -t typescript -t tsx # Look for the context definition echo "Checking AccountIconThemeContext definition..." rg "AccountIconThemeContext.*=.*createContext" -t typescript -t tsx # Check the storage implementation echo "Checking storage implementation..." rg "setStorage.*iconTheme" -t typescript -t tsxLength of output: 557
Script:
#!/bin/bash # Check theme storage and context implementation echo "Checking theme storage and context implementation..." rg "iconTheme" --glob "*.{ts,tsx}" # Look for the context definition echo "Checking AccountIconThemeContext definition..." rg "createContext.*AccountIconTheme" --glob "*.{ts,tsx}" # Check storage implementation echo "Checking storage implementation..." fd -e ts -e tsx -x cat {} | rg "setStorage"Length of output: 10390
packages/extension-polkagate/src/components/Identicon.tsx (2)
8-8: LGTM: Import statements are correctly organized.The new imports for
useContextandAccountIconThemeContextare properly placed and necessary for the theme context integration.Also applies to: 13-13
27-27: LGTM: Theme context integration is well implemented.The implementation correctly uses the React Context API with proper fallback handling:
- Uses
useContexthook according to React best practices- Maintains backward compatibility by falling back to
iconThemeprop- Preserves the existing Props interface, making it a non-breaking change
Let's verify that the AccountIconThemeContext provider is properly set up:
Also applies to: 37-37
✅ Verification successful
Based on the gathered information, I can now generate the final response as the context shows the complete implementation of the AccountIconThemeContext and its usage.
LGTM: Theme context is properly implemented and used
The AccountIconThemeContext is correctly set up and used throughout the application:
- Context is properly defined in
contexts.tsxwith correct typing- Provider is properly placed in the component tree in
Popup/index.tsx, wrapping all necessary components- Default theme value is set via
DEFAULT_ACCOUNT_ICON_THEMEconstant- Theme persistence is handled through storage in
SelectIdenticonTheme.tsx🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the AccountIconThemeContext provider setup # Expected: Find the context provider wrapping components that use Identicon # Search for AccountIconThemeContext provider usage ast-grep --pattern 'AccountIconThemeContext.Provider value={$_}' # Search for files importing and using Identicon to ensure they're under the provider rg -l "import.*Identicon.*from.*components/Identicon"Length of output: 122
Script:
#!/bin/bash # Let's try a broader search to find the context definition and usage # Find the AccountIconThemeContext definition rg -l "AccountIconThemeContext" # Find any Provider pattern usage in TSX/JSX files rg "Provider.*value=\{.*\}" --type tsx --type jsx -l # Look for Identicon imports with a more flexible pattern rg -l "Identicon" --type tsx --type jsx # Search for context creation ast-grep --pattern "createContext<$_>($_)"Length of output: 502
Script:
#!/bin/bash # Let's examine the context definition and its usage # Check the context definition file cat packages/extension-polkagate/src/components/contexts.tsx # Check how the context is used in the Popup component cat packages/extension-ui/src/Popup/index.tsx # Check the SelectIdenticonTheme component cat packages/extension-polkagate/src/components/SelectIdenticonTheme.tsx # Check the types definition cat packages/extension-polkagate/src/util/types.tsLength of output: 49144
packages/extension-polkagate/src/components/contexts.tsx (3)
6-6: LGTM! Clean import additionThe import statement follows the existing pattern and is properly grouped with related types.
32-32: LGTM! Clean export additionThe new context is properly exported and maintains the alphabetical ordering of exports.
29-29: LGTM! Verify context provider implementationThe context definition follows the established pattern and includes appropriate default values.
Let's verify that this context has a provider implementation:
✅ Verification successful
Context provider implementation verified ✓
The
AccountIconThemeContext.Provideris properly implemented inpackages/extension-ui/src/Popup/index.tsxwith both the theme value and setter being provided through the context value prop. The provider is also correctly positioned in the component hierarchy alongside other context providers.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for AccountIconThemeContext.Provider usage rg -l "AccountIconThemeContext.Provider" # Also check for potential wrapper components that might provide this context ast-grep --pattern 'const $_ = ({ children }) => { $$$ return <AccountIconThemeContext.Provider value={$_}> $$$ </AccountIconThemeContext.Provider> }'Length of output: 245
Script:
#!/bin/bash # Let's check the implementation in Popup/index.tsx rg -A 10 "AccountIconThemeContext.Provider" packages/extension-ui/src/Popup/index.tsxLength of output: 1251
packages/extension-polkagate/src/components/index.ts (1)
75-75: LGTM! Export follows established patterns.The new export for
SelectIdenticonThemeis well-placed alphabetically and follows the consistent pattern of default exports used throughout the file.packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/SettingSubMenuFullScreen.tsx (4)
12-12: LGTM! Import addition is properly organized.The SelectIdenticonTheme component import is correctly grouped with related components.
87-87: LGTM! Improved type safety with proper type assertion.The intermediate
unknownassertion before casting tobooleanis a safer approach when dealing with storage values of uncertain type.
137-140: LGTM! UI component placement maintains consistent layout.The SelectIdenticonTheme component is properly integrated with appropriate spacing and full width alignment.
137-139: Verify AccountIconThemeContext integration.Let's ensure the SelectIdenticonTheme component is properly connected to the AccountIconThemeContext.
packages/extension-polkagate/src/fullscreen/onboarding/index.tsx (2)
12-12: LGTM! Good practice moving the demo account to constants.Moving the demo account address to a constants file improves maintainability and follows the DRY principle.
Line range hint
54-58: LGTM! Clean implementation of the demo account constant.The implementation maintains proper error handling while using the centralized constant.
Let's verify the DEMO_ACCOUNT constant is properly defined:
✅ Verification successful
DEMO_ACCOUNT constant is properly defined and consistently used
The constant is centrally defined in
constants.tsxand is consistently imported and used across the codebase. It's used in:
- The onboarding flow for creating a demo account
- Select2 component for UI interactions
- No conflicting demo account constants were found
The implementation maintains clean code practices with proper centralization of constants.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify DEMO_ACCOUNT constant definition # Test: Search for DEMO_ACCOUNT definition rg -A 1 "DEMO_ACCOUNT.*="Length of output: 264
Script:
#!/bin/bash # Let's verify if this DEMO_ACCOUNT is used consistently across the codebase rg -A 2 "DEMO_ACCOUNT" # Also check if there are any other demo account constants that might conflict rg -i "demo.*account.*=.*"Length of output: 2123
packages/extension-polkagate/src/partials/SettingSubMenu.tsx (3)
14-14: LGTM: Import addition is clean and follows conventions.The
SelectIdenticonThemeimport is correctly added to the existing grouped import statement.
174-176: LGTM: SelectIdenticonTheme implementation aligns with PR objective.The component is well-positioned in the settings menu hierarchy and uses consistent styling with other settings components.
Line range hint
186-193: Verify the intent behind hiding the notification selector.While the notification selector is hidden but retained, this could indicate:
- A feature being gradually rolled out
- A feature being deprecated
- A temporary UI adjustment
Please clarify the reasoning to ensure this change is intentional and not an oversight.
✅ Verification successful
The previous command failed due to incorrect file type specification. Let's try with proper glob patterns to search for TypeScript and TSX files, and also look for the specific notification selector pattern with surrounding context.
Let's examine the related files to understand if this is a consistent pattern and what notification functionality is implemented across the application.
The notification selector is intentionally hidden in popup view but visible in fullscreen
Looking at both menu implementations:
SettingSubMenu.tsx(popup view): The notification selector is explicitly hidden withvisibility: 'hidden'SettingSubMenuFullScreen.tsx(fullscreen view): The notification selector is visible and fully functionalThis appears to be an intentional UI design choice where notification settings are only accessible in the fullscreen view of the application, likely to provide a better user experience in the limited space of the popup view.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any related changes to notification features rg -l "notification" --type ts --type tsxLength of output: 69
Script:
#!/bin/bash # Search for notification-related code with proper file extensions rg -l "notification" -g "*.ts" -g "*.tsx" # Search for specific notification selector pattern rg "notification.*Select" -g "*.ts" -g "*.tsx" -C 5Length of output: 561
Script:
#!/bin/bash # Let's examine the content of both setting menu files to understand the notification implementation cat packages/extension-polkagate/src/partials/SettingSubMenu.tsx cat packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/SettingSubMenuFullScreen.tsx # Also check the messaging file for notification-related functionality cat packages/extension-polkagate/src/messaging.tsLength of output: 24806
packages/extension-polkagate/src/components/Select2.tsx (3)
7-7: LGTM: New imports support identicon theming.The added imports are necessary for implementing the new identicon theme selection functionality.
Also applies to: 14-16
62-62: LGTM: Function signature with sensible defaults.The default values for the new props align well with the existing styling patterns.
Line range hint
1-224: Verify integration with AccountIconThemeContext.Let's verify that this component is properly integrated with the AccountIconThemeContext for theme selection.
✅ Verification successful
Let me generate one more verification script to check the complete integration flow:
This will help us verify the complete integration between Select2, SelectIdenticonTheme, and AccountIconThemeContext to make a final determination.
Let me try one final verification to locate the context file:
Based on the verification results, I can now provide the final response:
The Select2 component is correctly integrated with AccountIconThemeContext
The verification shows a proper implementation where:
- AccountIconThemeContext is properly defined with the required theme state and setter
- SelectIdenticonTheme component correctly:
- Consumes the context using useContext
- Passes the theme value to Select2 via the isIdenticon prop
- Updates the theme through the context's setter
- Select2 component properly renders different icon styles based on the isIdenticon prop
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the usage of Select2 component with AccountIconThemeContext # Find files that import both Select2 and AccountIconThemeContext echo "Checking component integration..." rg -l "Select2" | xargs rg "AccountIconThemeContext" # Find all usage of isIdenticon prop echo "Checking isIdenticon prop usage..." rg "isIdenticon" -A 5 -B 5Length of output: 5887
Script:
#!/bin/bash # Check the complete implementation of SelectIdenticonTheme echo "Checking SelectIdenticonTheme implementation..." cat packages/extension-polkagate/src/components/SelectIdenticonTheme.tsx # Check AccountIconThemeContext implementation echo -e "\nChecking AccountIconThemeContext implementation..." cat packages/extension-polkagate/src/components/contexts.tsLength of output: 1973
Script:
#!/bin/bash # Find the contexts file location echo "Searching for contexts file..." fd contexts -e ts -e tsx # Once found, check its content echo -e "\nSearching for AccountIconThemeContext definition..." rg -A 5 "AccountIconThemeContext" --type ts --type tsxLength of output: 356
Script:
#!/bin/bash # Check the contexts file content echo "Checking contexts.tsx content..." cat packages/extension-polkagate/src/components/contexts.tsx # Check all imports of Select2 to see its usage patterns echo -e "\nChecking all Select2 usages..." rg "import.*Select2" -A 2Length of output: 3846
packages/extension-polkagate/src/util/constants.tsx (1)
248-248: Verify the theme constant usage across components.The constant looks good and aligns with the PR objectives. Let's verify its usage across the codebase.
✅ Verification successful
The constant is properly used and integrated in the codebase
The verification shows that
DEFAULT_ACCOUNT_ICON_THEMEis:
- Imported and used correctly in
packages/extension-ui/src/Popup/index.tsxas a fallback value for account icon theme- Integrated with the
AccountIconThemeContextwhich is used throughout the application for consistent theming- The theme context is properly consumed by components using identicons, ensuring consistent icon appearance
The implementation is correct and there are no issues found with the constant's usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of DEFAULT_ACCOUNT_ICON_THEME constant # Test: Search for imports and usage of the constant rg "DEFAULT_ACCOUNT_ICON_THEME" -B 2 -A 2 # Test: Search for hardcoded 'polkadot' strings that might need to use the constant instead rg "'polkadot'" --type tsx --type tsLength of output: 1339
Script:
#!/bin/bash # Let's try a different approach to search for hardcoded 'polkadot' strings # and the usage of DEFAULT_ACCOUNT_ICON_THEME # Search for hardcoded 'polkadot' strings in all files rg -t ts -t tsx "polkadot" # Search for any theme-related settings or configurations rg -t ts -t tsx "iconTheme|theme.*polkadot" # Look for files that might be using account icons fd -e ts -e tsx | xargs rg "AccountIcon|identicon"Length of output: 19598
packages/extension-polkagate/src/util/types.ts (2)
14-14: LGTM: Clean import for icon theme typesThe import of
IconThemefrom '@polkadot/react-identicon/types' is properly placed with other type imports.
684-687: LGTM: Well-structured context type definitionThe
AccountIconThemeContextTypeinterface is well-defined with proper typing for both the theme state and its setter function, following React context patterns.packages/extension-ui/src/Popup/index.tsx (2)
7-7: LGTM: Required imports are properly added.The new imports for IconTheme type and DEFAULT_ACCOUNT_ICON_THEME constant are correctly placed and properly typed.
Also applies to: 87-87
305-396: LGTM: Context provider is properly integrated.The AccountIconThemeContext.Provider is:
- Correctly positioned high in the component tree
- Properly provides both theme state and setter
- Maintains logical nesting order with other providers
packages/extension/public/locales/en/translation.json (1)
1413-1414: LGTM! Translation keys added correctly.The new translation key "Account Icon" has been properly added, following the file's conventions and supporting the account icon theme feature.
packages/extension-polkagate/src/components/SelectIdenticonTheme.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/components/SelectIdenticonTheme.tsx
Outdated
Show resolved
Hide resolved
...ages/extension-polkagate/src/fullscreen/homeFullScreen/partials/SettingSubMenuFullScreen.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/extension-ui/src/Popup/index.tsx (6 hunks)
🔇 Additional comments (5)
packages/extension-ui/src/Popup/index.tsx (5)
7-7: Imported IconTheme for account icon theming
The import statement correctly adds IconTheme from @polkadot/react-identicon/types, which is necessary for managing account icon themes.
17-17: Included AccountIconThemeContext in imports
Adding AccountIconThemeContext ensures that the account icon theme context is available throughout the application.
87-87: Imported DEFAULT_ACCOUNT_ICON_THEME for default theme setting
The import of DEFAULT_ACCOUNT_ICON_THEME from constants provides a default value for the account icon theme.
307-308: Provided AccountIconThemeContext to the component tree
The AccountIconThemeContext.Provider is correctly added, allowing access to the account icon theme state throughout the application.
307-398: Verify the order of context providers for potential side effects
Ensure that the nesting order of the context providers does not introduce unintended side effects. If certain contexts depend on others, adjusting the order might be necessary to maintain the correct flow of data.
| @@ -137,6 +139,7 @@ export default function Popup (): React.ReactElement { | |||
| const [refs, setRefs] = useState<LatestRefs>({}); | |||
| const [accountsAssets, setAccountsAssets] = useState<SavedAssets | null | undefined>(); | |||
| const [currency, setCurrency] = useState<CurrencyItemType>(); | |||
| const [accountIconTheme, setAccountIconTheme] = useState<IconTheme>(); | |||
There was a problem hiding this comment.
Initialize accountIconTheme state with a default value
Currently, accountIconTheme is initialized without a default value, which may lead to it being undefined before the storage value is retrieved. This could cause issues in components that consume this context. Consider initializing it with DEFAULT_ACCOUNT_ICON_THEME to prevent potential rendering problems.
Apply this diff to initialize the state variable:
-const [accountIconTheme, setAccountIconTheme] = useState<IconTheme>();
+const [accountIconTheme, setAccountIconTheme] = useState<IconTheme>(DEFAULT_ACCOUNT_ICON_THEME);Committable suggestion was skipped due to low confidence.
# [0.21.0](v0.20.1...v0.21.0) (2024-10-29) ### Features * add account icon theme options ([#1612](#1612)) ([e1b3570](e1b3570))
Summary by CodeRabbit
Release Notes
New Features
SelectIdenticonThemecomponent for selecting account icon themes.IdenticonandSelect2components with improved theming and customization options.Improvements
SettingSubMenuandSettingSubMenuFullScreento includeSelectIdenticonThemefor better user interface.Localization
Bug Fixes
SettingSubMenuFullScreenfor improved type safety.