-
Notifications
You must be signed in to change notification settings - Fork 48
feat(why-kmesh): Add thematic sky-blue icons to feature cards #247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Yash Israni <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
❌ Deploy Preview for kmesh-net failed.
|
Summary of ChangesHello @yashisrani, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the "Why Kmesh" section by integrating thematic sky-blue icons into the feature cards. This change aims to improve the visual appeal and user experience by providing instant visual recognition of benefits, reducing cognitive load, and reinforcing brand identity through consistent design. The addition of these modern, clean icons contributes to a more engaging and aesthetically pleasing interface. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces icons to the feature cards in the 'Why Kmesh' section, which is a great visual enhancement. My review includes a few suggestions to improve maintainability and adhere to React and SCSS best practices. Specifically, I've recommended centralizing the icon styling in SCSS, using a more stable key for list rendering, and utilizing SCSS variables for colors.
</Translate> | ||
</> | ||
), | ||
icon: <Icon icon="mdi:connection" color={SKY_BLUE} width="24" height="24" />, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The color
, width
, and height
props on the Icon
component are redundant and potentially misleading, as these styles are already defined (and in the case of size, overridden) in the corresponding SCSS file (src/components/Why/index.scss
). The SCSS sets the width
and height
to 30px
(conflicting with the 24
here) and the fill
color. To improve maintainability and have a single source of truth for styling, it's best to remove these props and let CSS handle the presentation entirely. This change should be applied to all <Icon>
components in this file (lines 32, 41, 52, 65, 70).
icon: <Icon icon="mdi:connection" color={SKY_BLUE} width="24" height="24" />, | |
icon: <Icon icon="mdi:connection" />, |
</h1> | ||
<div className="reasonBoxContainer"> | ||
{reasons.map((item, index) => ( | ||
<div key={index} className="reasonBox"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the array index
as a key
is not recommended in React, as it can lead to performance issues and bugs with dynamic lists. Although this list is static, it's a good practice to use a unique and stable identifier from the data itself. The text content of the title
is a good candidate for a key.
<div key={index} className="reasonBox"> | |
<div key={item.title.props.children} className="reasonBox"> |
.reasonIcon { | ||
margin-bottom: 12px; | ||
display: flex; | ||
justify-content: center; | ||
align-items: center; | ||
width: 60px; | ||
height: 60px; | ||
background: rgba(79, 195, 247, 0.1); | ||
border-radius: 50%; | ||
transition: transform 0.2s ease; | ||
|
||
svg { | ||
width: 30px; | ||
height: 30px; | ||
fill: #4FC3F7; | ||
} | ||
|
||
&:hover { | ||
transform: scale(1.1); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The color #4FC3F7
(and its RGB equivalent) is used in multiple places. To improve maintainability and avoid magic values, it's a good practice to define it as a SCSS variable at the top of this class scope and reuse it. This makes it easier to update the theme color in the future.
.reasonIcon {
$sky-blue: #4FC3F7;
margin-bottom: 12px;
display: flex;
justify-content: center;
align-items: center;
width: 60px;
height: 60px;
background: rgba($sky-blue, 0.1);
border-radius: 50%;
transition: transform 0.2s ease;
svg {
width: 30px;
height: 30px;
fill: $sky-blue;
}
&:hover {
transform: scale(1.1);
}
}
Signed-off-by: Yash Israni <[email protected]>
Why this matters:
Before:
After: