-
Notifications
You must be signed in to change notification settings - Fork 1
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
Website about us profile update #237
Conversation
Reviewer's Guide by SourceryThis pull request updates the 'About Us' section of the website, focusing on enhancing the team profiles and improving the overall user interface. The changes include adding new social media prefixes, updating team member information, and refining the UI components for better consistency and readability. File-Level Changes
Tips
|
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.
Hey @tulsiojha - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Hard-coded LinkedIn URL prefix found. (link)
- Hard-coded GitHub URL prefix found. (link)
- Hard-coded Twitter URL prefix found. (link)
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🔴 Security: 3 blocking issues
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
toLabel="href" | ||
to={linkedInPrefix + t.linkedin} | ||
size="xs" | ||
target="__blank__" |
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.
issue (bug_risk): Incorrect target attribute value
The value for the target attribute should be "_blank" instead of "blank" to open the link in a new tab.
className="wb-h-full wb-w-full wb-object-cover" | ||
className={cn( | ||
'wb-h-full wb-w-full wb-object-cover', | ||
t.imageClassName, |
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.
issue (bug_risk): Check for undefined imageClassName
Ensure that t.imageClassName is always defined or provide a default value to avoid potential runtime errors.
src={aboutUsBanner.src} | ||
className="wb-h-full wb-w-full wb-object-cover" | ||
/> | ||
<div className="wb-absolute wb-inset-0 wb-bg-[#0000008c]" /> |
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.
suggestion: Consider using a CSS variable for the background color
Using a CSS variable for the background color would make it easier to maintain and update the color scheme across the application.
<div className="wb-absolute wb-inset-0 wb-bg-[#0000008c]" /> | |
:root { | |
--overlay-bg-color: #0000008c; | |
} | |
<div className="wb-absolute wb-inset-0 wb-bg-[var(--overlay-bg-color)]" /> |
@@ -109,7 +109,11 @@ const JoinProvidersDialog = ({ | |||
{oathProviders?.githubLoginUrl && ( | |||
<Button | |||
variant="tertiary" | |||
content="Continue with Github" | |||
content={ | |||
<span className="!wb-bodyLg"> |
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.
suggestion: Avoid using important flag in class names
Using the important flag in class names can lead to specificity issues and make the CSS harder to maintain. Consider refactoring the styles to avoid using it.
@@ -11,7 +12,13 @@ const AboutMain = () => { | |||
</div> | |||
<GraphExtended> | |||
<div className="wb-grid wb-grid-cols-1 wb-grid-rows-[352px_auto] md:wb-grid-cols-none wb-gap-3xl md:wb-gap-5xl"> | |||
<GraphItem className="wb-bg-surface-basic-subdued"> </GraphItem> | |||
<GraphItem className="wb-bg-surface-basic-subdued wb-relative"> | |||
<img |
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.
issue (complexity): Consider creating a separate ImageWithOverlay
component for the image and overlay logic.
The new code introduces increased complexity due to additional nesting and inline styles. Hardcoding styles directly in the JSX makes the code less maintainable and reduces reusability. To improve readability and maintainability, consider creating a separate ImageWithOverlay
component for the image and overlay logic. This approach will make the main component cleaner and more modular. Here's a refactored version:
import { GraphExtended, GraphItem } from '../../graph';
import SectionWrapper from '../section-wrapper';
import aboutUsBanner from '~/images/about-us/about-us-banner-1.jpg';
// Create a reusable component for the image with overlay
const ImageWithOverlay = ({ src, alt, overlayColor }) => (
<div className="wb-relative wb-h-full wb-w-full">
<img src={src} alt={alt} className="wb-h-full wb-w-full wb-object-cover" />
<div className="wb-absolute wb-inset-0" style={{ backgroundColor: overlayColor }} />
</div>
);
const AboutMain = () => {
return (
<SectionWrapper className="wb-flex wb-flex-col wb-w-full">
<div className="wb-flex wb-flex-col wb-gap-3xl wb-text-center">
<h1 className="wb-heading3xl-marketing md:wb-heading4xl-marketing lg:wb-heading5xl-marketing wb-text-text-default">
About us
</h1>
</div>
<GraphExtended>
<div className="wb-grid wb-grid-cols-1 wb-grid-rows-[352px_auto] md:wb-grid-cols-none wb-gap-3xl md:wb-gap-5xl">
<GraphItem className="wb-bg-surface-basic-subdued">
<ImageWithOverlay src={aboutUsBanner.src} alt="About Us Banner" overlayColor="#0000008c" />
</GraphItem>
<div className="wb-grid wb-grid-cols-1 md:wb-grid-cols-[448px_auto] wb-gap-3xl md:wb-gap-5xl xl:wb-h-[544px] 2xl:wb-h-[480px] 3xl:wb-h-[320px]">
<GraphItem className="wb-bg-surface-basic-subdued wb-p-5xl">
<span className="wb-text-text-default wb-headingXl-marketing md:wb-heading2xl-marketing lg:wb-heading3xl-marketing">
We make development a breeze, saving developers' time with our ultra-efficient platform
</span>
</GraphItem>
<GraphItem className="wb-bg-surface-basic-subdued wb-p-5xl">
<span className="wb-bodyLg md:wb-bodyXl lg:wb-bodyXXl wb-text-text-strong">
At Kloudlite, we understand the challenges developers face because we've experienced them ourselves. We know how precious your time is, so we created a platform to streamline and simplify the development process. Our mission is to provide you with the tools for a seamless, productive experience. With Kloudlite, you can easily integrate any Kubernetes cluster, whether on the cloud or locally. We're here to make development smoother, faster, and more enjoyable. Join us and let’s build something amazing together.
</span>
</GraphItem>
</div>
</div>
</GraphExtended>
</SectionWrapper>
);
};
export default AboutMain;
This refactor improves readability, reusability, and maintainability.
@@ -65,6 +69,10 @@ import TalescaleSvg from '../icons/Talescale'; | |||
import Button from '../components/button'; | |||
import JoinProvidersDialog from '../components/join-provider-dialog'; | |||
|
|||
export const linkedInPrefix = 'https://www.linkedin.com/in/'; |
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.
🚨 issue (security): Hard-coded LinkedIn URL prefix found.
Consider using environment variables to store URL prefixes to avoid hard-coding them in the source code.
@@ -65,6 +69,10 @@ import TalescaleSvg from '../icons/Talescale'; | |||
import Button from '../components/button'; | |||
import JoinProvidersDialog from '../components/join-provider-dialog'; | |||
|
|||
export const linkedInPrefix = 'https://www.linkedin.com/in/'; | |||
export const githubPrefix = 'https://github.com/'; |
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.
🚨 issue (security): Hard-coded GitHub URL prefix found.
Consider using environment variables to store URL prefixes to avoid hard-coding them in the source code.
@@ -65,6 +69,10 @@ import TalescaleSvg from '../icons/Talescale'; | |||
import Button from '../components/button'; | |||
import JoinProvidersDialog from '../components/join-provider-dialog'; | |||
|
|||
export const linkedInPrefix = 'https://www.linkedin.com/in/'; | |||
export const githubPrefix = 'https://github.com/'; | |||
export const twitterPrefix = 'https://x.com/'; |
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.
🚨 issue (security): Hard-coded Twitter URL prefix found.
Consider using environment variables to store URL prefixes to avoid hard-coding them in the source code.
Summary by Sourcery
This pull request updates the About Us page by adding personal website links for team members, enhancing social media link handling, and improving visual elements. It also refines text styling across various components and updates the navigation menu to use 'Docs' instead of 'Documentation'.