feat: override accent secondary with chain color #24
Conversation
WalkthroughThe recent updates focus on improving accent color management throughout the project. Changes include dynamic accent color setting based on the active chain ID in Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
rossbulat
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- src/config/networks/index.ts (5 hunks)
- src/library/HeaderMenu/Wrappers.ts (3 hunks)
- src/screens/Common/PageWithMenu/index.tsx (1 hunks)
Additional comments: 7
src/screens/Common/PageWithMenu/index.tsx (1)
- 16-24: The logic for determining the chain-specific color and applying it as an accent color is clear and straightforward. However, there are a few considerations and potential improvements:
Fallback Color Hardcoding: The fallback color (
#552bbf) is hardcoded within the component. It might be more maintainable to define this color in a centralized configuration or theme file. This approach would make it easier to update the color in the future and ensure consistency across the application.Error Handling: There's no explicit error handling if the
chainIddoes not exist within theNetworkDirectory. While the current logic defaults to the hardcoded fallback color, it might be beneficial to log an error or warning for debugging purposes, especially during development.Performance Consideration: The current implementation recalculates the
colorvariable on every render. If the calculation of the color based on thechainIdbecomes more complex in the future, consider memoizing thecolorvariable usinguseMemoto avoid unnecessary recalculations.Consider defining the fallback color in a centralized configuration or theme file for better maintainability. Additionally, evaluate the need for error handling and performance optimizations as the application evolves.
src/library/HeaderMenu/Wrappers.ts (4)
- 49-49: The update to use
--accent-color-secondaryfor the hover state of buttons within theHeaderMenuWrapperis consistent with the PR's objective to integrate chain-specific colors into the UI. This change enhances the visual appeal and consistency across different blockchain networks.- 52-52: The use of
--accent-color-secondaryfor the active state of buttons aligns with the PR's goal and ensures a cohesive user experience by reflecting the active blockchain network's color scheme.- 93-93: Similarly, the update in the
ButtonWrappercomponent to use--accent-color-secondaryfor the hover state of buttons is a good application of the PR's objectives, ensuring visual consistency across the application.- 103-103: Applying
--accent-color-secondaryfor the color of SVG icons in the active state of buttons within theButtonWrappercomponent is a thoughtful detail that contributes to the overall aesthetic and consistency of the UI.src/config/networks/index.ts (2)
- 17-17: The addition of the
colorfield to theNetworkDirectoryIteminterface is a crucial update that supports the PR's objective of integrating chain-specific colors into the UI. This change enables the dynamic application of accent colors based on the active blockchain network.- 31-31: Assigning specific color values to the different networks within the
NetworkDirectoryconstant is a foundational step for achieving visual consistency across blockchain networks. It's important to ensure that these color values are accurate representations of their respective networks to maintain the intended visual appeal and user experience.Also applies to: 50-50, 69-69, 80-80
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- src/theme/accents/developer-console.css (1 hunks)
- src/theme/accents/kusama-relay.css (1 hunks)
- src/theme/accents/polkadot-relay.css (1 hunks)
- src/theme/accents/westend-relay.css (1 hunks)
Files skipped from review due to trivial changes (2)
- src/theme/accents/developer-console.css
- src/theme/accents/polkadot-relay.css
Additional comments: 1
src/theme/accents/kusama-relay.css (1)
- 7-10: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-9]
The changes align with the PR's objectives by focusing on primary and secondary accent colors for the Kusama relay chain. As with the Westend relay theme, ensure that the removal of other accent colors does not negatively impact UI components that may depend on them.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- src/screens/Common/PageWithMenu/index.tsx (1 hunks)
- src/screens/Settings/Menu.tsx (1 hunks)
- src/theme/accents/developer-console.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- src/theme/accents/developer-console.ts
Files skipped from review as they are similar to previous changes (1)
- src/screens/Common/PageWithMenu/index.tsx
| style={Object.fromEntries([ | ||
| ['--accent-color-secondary', accentColors.primary.light], | ||
| ])} |
There was a problem hiding this comment.
The implementation of dynamic accent color application by overriding --accent-color-secondary with accentColors.primary.light is a good approach to ensure UI consistency with the active chain's color scheme. However, consider the following points for improvement:
-
Performance Consideration: Directly manipulating styles through inline styles can be less efficient than using CSS classes, especially if these styles need to be dynamically updated often. If the accent color changes frequently, consider using a CSS class and modifying the class based on the active chain.
-
Maintainability: Inline styles can sometimes make the code harder to maintain, especially if there are multiple places where the accent color needs to be overridden. It might be beneficial to centralize the styling logic or use a context/provider to manage the theme colors dynamically across the application.
-
Fallback Value: Ensure there's a fallback value for
accentColors.primary.lightin case it's undefined for some reason. This helps in maintaining a consistent UI experience even when unexpected issues arise.
| entries?: Record<string, string> | ||
| ) => { | ||
| const result = entries; | ||
| let updated = false; | ||
| const updated = false; | ||
|
|
||
| if (result) { | ||
| Object.entries(result).forEach(([tabId, entry]) => { | ||
| if (!activeTabs?.find(({ id }) => id === Number(tabId)) || entry === '') { | ||
| delete result[Number(tabId)]; | ||
| updated = true; | ||
| } | ||
| }); | ||
| } | ||
| const result = Object.fromEntries( | ||
| Object.entries(entries || {}).reduce( | ||
| (acc: [string, string][], [tabId, entry]) => { | ||
| if (activeTabs.find(({ id }) => id === Number(tabId)) && entry !== '') { | ||
| return acc.concat([[tabId, entry]]); | ||
| } | ||
| return acc; | ||
| }, | ||
| [] | ||
| ) | ||
| ); |
There was a problem hiding this comment.
The implementation of sanitizeKeysForTabExistence using Object.fromEntries and reduce is correct and efficient for filtering and constructing the result object based on the existence of tabs. However, the variable updated is initialized but never changed within the function, which might indicate an oversight or unfinished logic. Consider if the updated flag should reflect changes made during the sanitization process.
| @@ -109,7 +110,7 @@ export const sanitizeTags = ({ | |||
| for (const chain of chains) { | |||
| if (!NetworkDirectory[chain]) { | |||
| // If error, fallback to defaults if exist, otherwise filter out the chain. | |||
| result[tagId] = | |||
| result.tagId = | |||
There was a problem hiding this comment.
In sanitizeTags, the assignment to result.tagId seems incorrect. This line attempts to assign a value to a property named tagId on the result object, which does not dynamically reference the current tagId being iterated over. Instead, it should use bracket notation to correctly assign the value to the dynamic key represented by tagId.
- result.tagId =
+ result[tagId] =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.
| result.tagId = | |
| result[tagId] = |
| const updated = false; | ||
|
|
||
| if (result) { | ||
| Object.entries(appliedTags).forEach(([tabId, applied]) => { | ||
| if (!activeTabs?.find(({ id }) => id === Number(tabId))) { | ||
| delete result[Number(tabId)]; | ||
| updated = true; | ||
| } else { | ||
| const result = Object.fromEntries( | ||
| Object.entries(appliedTags || {}).reduce( | ||
| (acc: [string, string[]][], [tabId, applied]) => { | ||
| if (!activeTabs?.find(({ id }) => id === Number(tabId))) { | ||
| return acc; | ||
| } | ||
| // Check if each `applied` value is a valid tag id, and remove otherwise. | ||
| applied.forEach((tagId) => { | ||
| applied = applied.reduce((acc2: string[], tagId) => { | ||
| if (!tags?.[tagId]) { | ||
| result[Number(tabId)].splice(applied.indexOf(tagId), 1); | ||
| updated = true; | ||
| return acc2; | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
| return acc2.concat(tagId); | ||
| }, []); | ||
|
|
||
| return acc.concat([[tabId, applied]]); | ||
| }, | ||
| [] | ||
| ) | ||
| ); |
There was a problem hiding this comment.
The sanitizeAppliedTags function correctly uses Object.fromEntries and reduce to filter and construct the result object. The logic for filtering out invalid tab IDs and ensuring that each applied value is a valid tag ID is implemented correctly. However, similar to sanitizeKeysForTabExistence, the updated variable is initialized but never modified. If the intention is to track whether any changes have been made during the sanitization process, ensure that the updated flag is appropriately updated.
Summary by Ross
Adds chain colors and overrides
--accent-color-secondaryfor chain UI.Summary by CodeRabbit
PageWithMenucomponent to dynamically set accent colors based on the active chain ID.Menu.tsxfile to match the primary color.