-
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
Devdoc/UI #235
Devdoc/UI #235
Conversation
… join dialog box width issue
Reviewer's Guide by SourceryThis pull request introduces significant updates to the 'about us' section, including new team members, investors, mission, and vision statements. It also enhances the search functionality, updates various page components, and makes numerous styling and layout adjustments. Additionally, it includes ESLint configuration updates and the removal of unused components. 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 they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 3 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -103,6 +103,8 @@ const loadIndexesImpl = async ( | |||
}, | |||
}); | |||
|
|||
console.log('searchData', sectionIndex); |
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: Remove console.log statement
Consider removing the console.log
statement to avoid unnecessary logging in production.
console.log('searchData', sectionIndex); | |
// console.log('searchData', sectionIndex); |
@@ -246,13 +250,20 @@ | |||
} | |||
return a._page_rk - b._page_rk; | |||
}) | |||
.filter( | |||
(res) => | |||
(res.route.startsWith('/docs') && |
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.
question: Potentially redundant filter condition
The filter condition for routes starting with '/docs' or '/blog' might be redundant if the search results are already scoped to these routes. Verify if this additional filtering is necessary.
}; | ||
}, []); | ||
|
||
const showSearch = () => |
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: Function could be simplified
The showSearch
function could be simplified to a single return statement: const showSearch = () => route.route.startsWith('/blog') || route.route.startsWith('/docs');
); | ||
}, [config]); | ||
|
||
return ( | ||
<div className="wb-flex wb-flex-col"> | ||
<div className="wb-py-6xl md:wb-py-8xl lg:wb-py-10xl wb-flex wb-flex-col"> | ||
<div className="wb-flex wb-flex-col wb-gap-3xl"> | ||
<div className="wb-flex wb-flex-col wb-gap-2xl"> |
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: Consistent spacing
The change from wb-gap-3xl
to wb-gap-2xl
improves the visual consistency of the blog page. Ensure this change aligns with the overall design guidelines.
<div className="wb-flex wb-flex-col wb-gap-2xl"> | |
<div className="wb-flex wb-flex-col wb-gap-2xl md:wb-gap-3xl lg:wb-gap-4xl"> |
@@ -61,6 +62,7 @@ const Illustration = () => { | |||
<Wrapper className="-wb-mt-5xl"> | |||
<div className="hidden md:block wb-pb-[36px] 2xl:wb-pb-[128px]"> | |||
<GraphExtended | |||
// graph="graphIllustration" |
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: Remove commented code
Consider removing the commented-out graph
prop to keep the code clean and maintainable.
// graph="graphIllustration" | |
<Wrapper className="-wb-mt-5xl"> | |
<div className="hidden md:block wb-pb-[36px] 2xl:wb-pb-[128px]"> | |
<GraphExtended | |
innerClass="md:-wb-mx-[32px] lg:-wb-mx-[160px] -wb-mt-[2px] wb-flex wb-justify-center !wb-pt-[32px] 3xl:-wb-mx-[256px]" | |
className="!wb-pb-[32px] xl:[background-position: unset] 2xl:[background-position:top] 3xl:[background-position:unset]" |
@@ -52,14 +54,14 @@ const DontBelieve = () => { | |||
<Block title="Don't believe? Read for yourself.."> | |||
<div className="wb-block md:wb-hidden"> | |||
<Slider autoPlay active={`${active}`} onMove={(e) => setActive(e)}> | |||
{consts.home.messages.map((message) => ( | |||
{consts.homeNew.messages.map((message) => ( |
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 keeping the data source consistent.
The new code introduces some complexity that could be improved. Here are a few suggestions:
-
Inconsistent Data Source: The change from
consts.home.messages
toconsts.homeNew.messages
introduces a new data source. This can lead to confusion and potential bugs if the two data sources are not kept in sync or if the developer is not aware of the change. Consider keeping the data source consistent. -
Redundant Code: The addition of
tabIndex={-1}
to theProfile
components is repeated in both the light and dark mode spans. This redundancy can be avoided by refactoring the code to reduce duplication. AProfileWrapper
component can be created to handle this. -
Inline Function in JSX: The inline function in the
Slider
component (onMove={(e) => setActive(e)}
) can be extracted to a separate function to improve readability and maintainability.
Here is a refactored version of the code that addresses these issues:
const MessageCard = ({ title, subtitle, message }) => (
<div className="xl:wb-min-h-[320px] 2xl:wb-max-h-[288px] 3xl:wb-min-h-[256px] wb-flex wb-flex-col wb-gap-3xl wb-p-3xl wb-h-full wb-bg-surface-basic-default wb-min-h-[224px]">
<div className="wb-flex wb-flex-row wb-items-center wb-gap-3xl">
<ProfileWrapper title={title} subtitle={subtitle} color="one" className="wb-flex-1 dark-hidden" />
<ProfileWrapper title={title} subtitle={subtitle} color="dark" className="wb-flex-1 wb-hidden dark-block" />
</div>
<p className="wb-bodyLg wb-text-text-soft">{message}</p>
</div>
);
const ProfileWrapper = ({ title, subtitle, color, className }) => (
<span className={className}>
<Profile
responsive={false}
name={<span className="wb-bodyLg-medium">{title}</span>}
subtitle={<span className="wb-bodyMd">{subtitle}</span>}
color={color}
size="md"
noImage
tabIndex={-1}
/>
</span>
);
const DontBelieve = () => {
const [active, setActive] = useState(0);
const handleMove = (e) => {
setActive(e);
};
return (
<Block title="Don't believe? Read for yourself..">
<div className="wb-block md:wb-hidden">
<Slider autoPlay active={`${active}`} onMove={handleMove}>
{consts.home.messages.map((message) => (
<MessageCard key={message.title} {...message} />
))}
</Slider>
</div>
<div className="wb-h-full wb-hidden md:wb-grid wb-grid-cols-1 md:wb-grid-cols-3 wb-gap-3xl lg:wb-gap-5xl">
{consts.home.messages.map((message) => (
<GraphItem key={message.title}>
<MessageCard {...message} />
</GraphItem>
))}
</div>
</Block>
);
};
export default DontBelieve;
These changes should make the code easier to maintain and understand.
@@ -47,6 +48,23 @@ function GitTimestamp({ timestamp }: { timestamp: Date }) { | |||
); | |||
} | |||
|
|||
const _ProductHuntStatus = () => { |
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 moving the _ProductHuntStatus
component to a separate file.
The new code introduces additional complexity for several reasons:
- Inline Component Definition: The
_ProductHuntStatus
component is defined inline, increasing cognitive load. Consider moving it to a separate file to improve maintainability. - Additional Imports: The import for
Flexsearch
adds to the number of dependencies, making the file more complex. - Conditional Rendering Changes: Modifications to the conditional rendering logic, such as for the
GitTimestamp
component, can introduce subtle bugs and make the code harder to follow. - Meta Tag Changes: The changes to how the meta description is set add another layer of logic that needs to be understood.
- Formatting and Syntax: Minor formatting and syntax changes, like added commas and reformatting of JSX attributes, can reduce readability.
To simplify, consider modularizing components and maintaining a clean structure. For example, move _ProductHuntStatus
to a separate file and import it. This will make the main file cleaner and easier to navigate.
<ButtonGroup.IconButton value="light" icon={<Sun />} /> | ||
<ButtonGroup.IconButton value="dark" icon={<Moon />} /> | ||
<ButtonGroup.IconButton value="system" icon={<Monitor />} /> | ||
<ButtonGroup.IconButton |
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 reducing redundancy in the ButtonGroup.IconButton components.
The new code introduces some complexity that could be streamlined. Here are a few suggestions:
-
Increased Line Count and Redundancy: The new code has additional lines and redundancy, especially in the
ButtonGroup.IconButton
components. Consider using an array to map over and create these components to reduce redundancy. -
Hardcoding Strings: Strings like
siteTitle
andsiteDesc
are hardcoded directly in the export. These should ideally be constants or come from a configuration file to make future updates easier and less error-prone. -
Inconsistent Formatting: The new code has inconsistent formatting, such as trailing commas and spacing. Consistent formatting improves readability and maintainability.
-
Unnecessary Changes: Some changes, like moving the
Contact us
link, seem unnecessary and add to the complexity without clear benefit.
Here is a refactored version that addresses these issues:
import {
BrandLogo,
Monitor,
Moon,
Sun,
TwitterNewLogoFill,
} from '~/app/icons/icons';
import OssIcon from '~/images/homeNew/oss.svg';
import OssIconDark from '~/images/homeNew/oss-dark.svg';
import { cn } from './commons';
import { IConfig } from './use-config';
import { useTheme } from './useTheme';
export const siteDesc =
'Kloudlite is a remote-local development environment platform designed to streamline the workflow for developers working on distributed applications. By integrating both local and remote environments through Kubernetes, Kloudlite ensures a seamless, productive, and more connected development experience.';
export const basePath = 'https://kloudlite.io';
export const authUrl = 'https://auth.kloudlite.io';
export const gitUrl = 'https://github.com/kloudlite/kloudlite';
export const communityUrl =
'https://github.com/kloudlite/kloudlite/discussions';
const linkedinUrl = 'https://linkedin.com/company/kloudlite-io';
const xUrl = 'https://x.com/kloudlite';
export const supportEmail = '[email protected]';
const socialIconSize = 18;
const BrandMenu = ({ className }: { className?: string }) => {
const brandIconSize = 28;
const { theme, setTheme } = useTheme();
const themeButtons = [
{ value: 'light', icon: <Sun />, label: 'light-theme' },
{ value: 'dark', icon: <Moon />, label: 'dark-theme' },
{ value: 'system', icon: <Monitor />, label: 'system-theme' },
];
return (
<div
className={cn(
'wb-flex wb-flex-col wb-gap-7xl md:wb-gap-3xl lg:wb-pr-4xl wb-order-last md:wb-order-first md:wb-justify-between md:wb-h-full',
className
)}
>
<div className="wb-flex wb-flex-row">
<div className="wb-flex wb-flex-col wb-gap-3xl wb-flex-1 lg:wb-min-h-[200px] xl:wb-min-h-[236px] wb-items-start">
<div className="wb-flex wb-flex-col wb-items-start wb-gap-xl wb-max-w-[300px]">
<a href="/" aria-label="kloudlite">
<BrandLogo size={brandIconSize} detailed />
</a>
<span className="wb-bodyMd wb-text-text-soft">
Boost your efficiency, speed up deployments, enhance collaboration
</span>
</div>
<ButtonGroup.Root
variant="outline"
selectable
value={theme}
onValueChange={(v: any) => {
setTheme(v);
}}
>
{themeButtons.map((btn) => (
<ButtonGroup.IconButton
key={btn.value}
value={btn.value}
icon={btn.icon}
aria-label={btn.label}
/>
))}
</ButtonGroup.Root>
</div>
<div className="wb-bodyMd wb-text-text-soft wb-hidden md:wb-flex lg:wb-hidden wb-flex-col wb-gap-3xl wb-items-end md:wb-self-end lg:wb-self-auto">
<SocialMenu />
<div>© {new Date().getFullYear()} Kloudlite Labs Pvt Ltd.</div>
</div>
</div>
<div className="wb-bodyMd wb-text-text-soft wb-flex md:wb-hidden lg:wb-flex wb-flex-col wb-gap-3xl">
<SocialMenu />
<div>© {new Date().getFullYear()} Kloudlite Labs Pvt Ltd.</div>
</div>
</div>
);
};
export default {
siteTitle: 'Kloudlite - Development Environment as a Service',
logo: (
<Link href="/" aria-label="Kloudlite homepage">
<BrandLogo detailed size={28} />
</Link>
),
footer: {
brand: <BrandMenu className="md:wb-order-[-9999]" />,
extra: (
<div>
<img className="dark-hidden" src={OssIcon.src} alt="oss-light" />
<img className="wb-hidden dark-block" src={OssIconDark.src} alt="oss-dark" />
</div>
),
links: [
{
title: 'Product',
className: 'wb-basis-1/2 md:wb-basis-auto wb-flex',
showExtra: true,
items: [
{ title: 'Features', to: '/features' },
{ title: 'Pricing', to: '/pricing' },
{ title: 'Docs', to: '/docs' },
],
},
{
title: 'Resources',
className: 'wb-basis-1/2 md:wb-basis-auto wb-flex',
showExtra: false,
items: [
{ title: 'About us', to: '/about-us' },
{ title: 'Blog', to: '/blog' },
],
},
{
title: 'Company',
className: 'wb-basis-1/2 md:wb-basis-auto wb-flex',
showExtra: true,
items: [
{ title: 'Contact us', to: '/contact-us' },
{ title: 'Terms of services', to: '/terms-of-services' },
{ title: 'Privacy policy', to: '/privacy-policy' },
],
},
],
social: [
{ title: 'GitHub', type: 'normal', to: gitUrl },
{ title: 'LinkedIn', type: 'normal', to: linkedinUrl },
{ title: 'X', type: 'normal', to: xUrl },
{ title: 'Community', type: 'normal', to: communityUrl },
{ title: 'Pricing', type: 'normal', to: '/pricing' },
],
},
urls: {
auth: 'auth.kloudlite.io',
console: 'console.kloudlite.io',
},
} as IConfig;
Key Improvements:
- Reduced Redundancy: The
themeButtons
array is used to map over and create theButtonGroup.IconButton
components, reducing redundancy. - Consistent Formatting: Ensured consistent formatting and spacing.
- Centralized Strings: Kept strings like
siteTitle
andsiteDesc
centralized for easier updates. - Logical Grouping: Grouped related items together for better readability and maintainability.
Summary by Sourcery
This pull request introduces a new 'About Us' page, updates the pricing page, enhances the search functionality, and improves the header and footer components. It also includes updates to the privacy policy and terms of services pages, adds new tests, and removes deprecated components.