Skip to content
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

Refactor Code and Implement details and list page for httpproxy #2

Open
wants to merge 139 commits into
base: main
Choose a base branch
from

Conversation

meysam-jeffrey
Copy link
Contributor

Objective

  • implement details route page
  • refactor create and edit component route
  • implement List page of HTTPProxy routes
  • change and refactor strucutre project
  • add chart for metric data

@meysam-jeffrey meysam-jeffrey added the good first issue Good for newcomers label Jan 13, 2025
@meysam-jeffrey meysam-jeffrey self-assigned this Jan 13, 2025
@MortezaMirjavadi
Copy link
Collaborator

@meysam-jeffrey Jan, if you possible, attach a demo for changes.

@MortezaMirjavadi
Copy link
Collaborator

@meysam-jeffrey Jan, I think it's better to use yarn and remove of the package-lock.json, what do you think?

@MortezaMirjavadi
Copy link
Collaborator

Using sub-components in large React components is a best practice that leads to:

  • More readable code: Easier to understand and reason about.
  • More maintainable code: Easier to change and debug.
  • More reusable code: Components can be used in other parts of the application.
  • Better organized code: Clear separation of concerns.
  • Improved collaboration: Easier for teams to work together.
  • Potential performance benefits: More efficient rendering.

@meysam-jeffrey

src/components/chart/chart.tsx Outdated Show resolved Hide resolved
Comment on lines 49 to 57
<ChartLine
key={series.name}
data={series.data}
style={{
data: {
stroke: series.color,
},
}}
/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@meysam-jeffrey Jan, my suggestion is to extract this implementation to another sub-component like this:

interface ChartSeriesProps {
  series: {
    name: string;
    data: { x: number; y: number }[];
    color: string;
  };
}

const ChartSeries: React.FC<ChartSeriesProps> = ({ series }) => {
  const lineStyle = {
    data: {
      stroke: series.color,
    },
  };

  return <ChartLine key={series.name} data={series.data} style={lineStyle} />;
};

and finally the usage of this component like this:

<ChartGroup>
            {chartData.map((series: any, index) => (
              <ChartSeries key={series.name} series={series} />
            ))}
</ChartGroup>

Comment on lines 2 to 19
export interface ChartMetricProps {
title: string;
name: string;
chartData: [];
width?: number;
height?: number;
legendPosition?: ChartProps['legendPosition'];
yAxisTickValues?: number[];
xAxisTickValues?: (string | number)[];
themeColor?;
showGrid?: boolean;
padding?: {
top?: number;
bottom?: number;
left?: number;
right?: number;
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@meysam-jeffrey Jan, there is no any dependency between props?

Comment on lines 32 to 34
useEffect(() => {
onChange(formData);
}, [formData]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 37 to 40
updateFormData((prev) => ({
...prev,
routes: [...prev.routes, { ...DEFAULT_ROUTE }],
}));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion is to use Immerjs for better readability and writability.
https://immerjs.github.io/immer/

<div className="pf-u-pt-md">
<Flex justifyContent={{ default: 'justifyContentSpaceBetween' }}>
<FlexItem>
{onDelete && (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better using ternary operator instead of and-sign.

Comment on lines 71 to 93
<div key={index} className="pf-u-mb-md">
<div>
{route.services.length > 1 && (
<Button
className="delete-button"
variant="link"
onClick={() => removeService(index)}
>
<span className="fa fa-minus-circle pf-u-mr-xs"></span>
{t('remove_service')}
</Button>
)}
<ServiceForm
service={service}
onChange={(updatedService) =>
updateService(index, updatedService)
}
availableServices={availableServices}
availablePorts={availablePorts}
availableSecrets={availableSecrets}
/>
</div>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to extract in a sub-component for better readability.

Comment on lines 110 to 133
<Flex>
<FlexItem>
<FormGroup fieldId="idle" label="Idle Connection Timeout (seconds)">
<TextInput
type="number"
value={service.idleConnection}
onChange={(value) =>
onChange({ ...service, idleConnection: value })
}
/>
</FormGroup>
</FlexItem>
<FlexItem>
<FormGroup fieldId="response" label="Response Timeout (minutes)">
<TextInput
type="number"
value={service.responseTimeout}
onChange={(value) =>
onChange({ ...service, responseTimeout: value })
}
/>
</FormGroup>
</FlexItem>
</Flex>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to move it to a sub-component for better readability.

@@ -0,0 +1,41 @@
import * as React from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@meysam-jeffrey Jan, if you possible add rule in webpack config that we don't need import react each file.

add plugins in webpack

import { ProvidePlugin } from 'webpack';
...
 plugins: [
    new ProvidePlugin({
      React: 'react',
    }),
...

add these two lines in tsconfig:

 "jsx": "react-jsx",
    "jsxImportSource": "react"

and finally add this section to the eslint.yml:

plugins:
  - prettier
  - react
  - '@typescript-eslint'
rules:
  prettier/prettier:
    - error
  react/jsx-uses-react: 'off'
  react/jsx-uses-vars: 'warn'
  react/prop-types: 'off'
settings:
  react:
    version: detect

Comment on lines 95 to 138
<Grid hasGutter={true}>
<GridItem span={6}>
<Text className="pf-u-mt-xl">
<strong>{t('details_name')}</strong>
</Text>
<Text>{router?.metadata?.name}</Text>
<Text className="pf-u-mt-xl">
<strong>{t('details_namespace')}</strong>
</Text>
<Text>
<Badge className="pf-u-mr-xs">{t('ns')}</Badge>
{router?.metadata?.namespace}
</Text>
<Text className="pf-u-mt-xl">
<strong>{t('details_created')}</strong>
</Text>
<Text>{router?.metadata?.creationTimestamp}</Text>
</GridItem>
<GridItem span={6}>
<Text className="pf-u-mt-xl">
<strong>{t('location')}</strong>
</Text>
<Text>
<a target="_blank" href={location}>
{location}
</a>
</Text>
<Text className="pf-u-mt-xl">
<strong>{t('details_status')}</strong>
</Text>
<Badge className="pf-u-mr-xs">{t('accepted')}</Badge>
<Text className="pf-u-mt-xl">
<strong>{t('details_fqdn')}</strong>
</Text>
<Text>{router?.spec?.virtualhost?.fqdn}</Text>
<Text className="pf-u-mt-xl">
<strong>{t('router_canonical')}</strong>
</Text>
<Text>
{router?.status?.loadBalancer?.ingress?.[0]?.ip ||
'-'}
</Text>
</GridItem>
</Grid>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move it to a sub-component?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants