Sky Upgrade page banner update #4033
Conversation
Use multiple CTAs on the upgrade banner
WalkthroughThe changes introduce a new type definition Changes
Possibly related PRs
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: 2
🧹 Outside diff range and nitpick comments (2)
pages/[networkOrProduct]/sky/upgrade.tsx (2)
Line range hint
80-102: LGTM! Consider extracting CTA configuration for improved readability.The changes to the
ActionBannercomponent successfully implement multiple CTA buttons as per the PR objectives. The updated title and description accurately reflect the new options for users to earn SKY rewards or Chronicle Points.To improve code readability and maintainability, consider extracting the CTA configuration into a separate constant:
const ctaConfig = [ { label: 'Get SKY Rewards', url: `/earn/srr/${wallet?.accounts[0].address}`, }, { label: 'Earn Chronicle Points', url: `/earn/cle/${wallet?.accounts[0].address}`, }, ]; // Then in the ActionBanner component: cta={ctaConfig}This refactoring would make it easier to manage and update the CTA options in the future.
🧰 Tools
Biome
[error] 78-78: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
1-153: Overall implementation looks good. Consider updating comments for consistency.The changes to the
ActionBannercomponent have been well-implemented and don't negatively impact the rest of the file. TheSkySwapWrapperfunction still correctly checks for a non-zero USDS balance before rendering the banner.For consistency and to help future developers, consider updating the comments at the top of the file to reflect the new functionality of multiple CTAs and the option to earn either SKY rewards or Chronicle Points. This will make it easier for other developers to understand the purpose and functionality of this component at a glance.
🧰 Tools
Biome
[error] 78-78: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- components/ActionBanner.tsx (3 hunks)
- pages/[networkOrProduct]/sky/upgrade.tsx (2 hunks)
🔇 Additional comments not posted (3)
pages/[networkOrProduct]/sky/upgrade.tsx (1)
86-86: Verify privacy implications of using wallet addresses in URLs.The implementation uses the user's wallet address in the URLs for the CTA buttons. While this is a common practice, it's important to ensure that this doesn't lead to any privacy concerns or unintended information exposure.
Please confirm that:
- The use of wallet addresses in these URLs is necessary for the functionality.
- There are no privacy implications or risks associated with exposing these addresses in the URL.
- The pages these URLs lead to have appropriate access controls to prevent unauthorized access to user data.
You can use the following script to check for other instances of wallet addresses being used in URLs throughout the codebase:
This will help ensure consistent and secure handling of wallet addresses across the application.
Also applies to: 90-90
components/ActionBanner.tsx (2)
42-94: Extraction ofCtaComponentenhances modularityIntroducing the
CtaComponentencapsulates the CTA rendering logic, improving code modularity and readability. This makes the codebase cleaner and theActionBannercomponent more maintainable.
144-144: Effective integration ofCtaComponentinActionBannerThe integration of
CtaComponentwithinActionBannerensures consistent rendering of CTAs and leverages the newly created component effectively.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
features/homepage/helpers/constants.ts (1)
Line range hint
1-41: Consider standardizing logo sizes and adding URLsWhile the changes look good, I have two suggestions for improving the overall
partnerLogosConfig:
Empty URLs: All entries, including the new 'Sky' entry, have empty URL fields. Consider adding appropriate URLs for each partner to enhance user interaction and provide more information about the partners.
Logo size consistency: The dimensions of the logos vary significantly across entries. For example, the new 'Sky' logo (110x38px) is considerably smaller than some others like 'Aave' (146x100px) or 'Ajna' (163x30px). Consider standardizing the logo sizes or at least setting a consistent maximum width or height to improve the visual consistency of the slider.
Here's a suggestion to improve consistency:
export const partnerLogosConfig: ImagesSliderProps['items'] = [ { imgSrc: staticFilesRuntimeUrl('/static/img/homepage/slider/aave.svg'), imgAlt: 'Aave', url: 'https://aave.com', // Add appropriate URL maxWidth: '160px', // Standardize max width height: 'auto', }, // ... (apply similar changes to other entries) { imgSrc: staticFilesRuntimeUrl('/static/img/homepage/slider/sky.svg'), imgAlt: 'Sky', url: 'https://sky.xyz', // Add appropriate URL maxWidth: '160px', // Standardize max width height: 'auto', }, // ... (apply similar changes to remaining entries) ]This approach would help maintain visual consistency while allowing for different aspect ratios of the logos.
theme/icons/cle.tsx (1)
16-16: Ensure unique SVGidattributes to prevent potential collisionsThe
idattributeclip0_15346_4406may cause collisions if multiple instances of this icon are rendered on the same page. Consider generating a uniqueidor making it more descriptive to avoid conflicts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (2)
public/static/img/homepage/slider/sky.svgis excluded by!**/*.svgpublic/static/img/protocol_icons/sky_logo.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
- features/homepage/helpers/constants.ts (1 hunks)
- lendingProtocols/lendingProtocolsConfigs.ts (2 hunks)
- theme/icons/cle.tsx (1 hunks)
🔇 Additional comments not posted (3)
features/homepage/helpers/constants.ts (1)
20-24: Logo update: Maker replaced with SkyThe changes look good overall. The 'Maker' logo has been replaced with 'Sky', which aligns with the PR objective of updating the Sky Upgrade page banner. A few observations:
- The image source has been correctly updated to point to the Sky logo.
- The alt text has been appropriately changed to 'Sky'.
- The dimensions have been adjusted, likely to accommodate the new logo's aspect ratio.
To ensure the new logo file exists and the dimensions are correct, please run the following script:
This script will help verify the existence of the new logo file and, if possible, check its dimensions to ensure they match the values specified in the code.
lendingProtocols/lendingProtocolsConfigs.ts (2)
13-13: LGTM: New logo import added correctly.The addition of the
skyLogoimport is consistent with the existing import pattern and aligns with the PR objective of updating the Sky Upgrade page banner.
Line range hint
1-100: Overall assessment: Changes look good and align with PR objectives.The updates to the Sky protocol configuration, including the new logo import and its usage, are well-implemented and consistent with the goal of enhancing the upgrade banner. The changes maintain the existing code structure and type safety. No major issues were identified during the review.
Use multiple CTAs on the upgrade banner
Refactor ActionBanner component to support multiple CTAs
Summary by CodeRabbit
cleicon for a more modern appearance.