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.

Sign up for GitHub

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

Feat/fe/infrastructure details connect to be #1172

Merged
merged 6 commits into from
Nov 22, 2024
Merged

Feat/fe/infrastructure details connect to be #1172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
90acb72
- Connect to BE to get infrastructure details
jennifer-gan Nov 20, 2024
afee715
Merge branch 'develop' into feat/fe/infrastructure-details-connect-to-BE
jennifer-gan Nov 20, 2024
d12420e
Merge branch 'develop' into feat/fe/infrastructure-details-connect-to-BE
jennifer-gan Nov 20, 2024
e239a72
- cleanup
jennifer-gan Nov 20, 2024
a0694d4
Merge branch 'develop' into feat/fe/infrastructure-details-connect-to-BE
jennifer-gan Nov 20, 2024
3053d63
- extend field class instead
jennifer-gan Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions Client/src/Components/Inputs/Field/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,9 @@
min-width: var(--env-var-width-3);
}

.field-infrastructure-alert {
.field-infrastructure-alert{
max-width: var(--env-var-width-4);
}

.field-infrastructure-alert .MuiInputBase-root:has(input) {
height: var(--env-var-height-2);
min-width: unset;
}

.field h3.MuiTypography-root,
Expand Down
6 changes: 3 additions & 3 deletions Client/src/Components/Inputs/Field/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ const Field = forwardRef(
return (
<Stack
gap={theme.spacing(2)}
className={`${className ?? `field field-${type}`}`}
className={`field field-${type} ${className}`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jennifer-gan I'm really glad to see you picked up on this issue too! I just submitted a PR that does basically this, could you remove this from this PR so that this one is focused on the details page please? One feature or fix per PR 😄

sx={{
"& fieldset": {
borderColor: theme.palette.border.dark,
Expand Down Expand Up @@ -186,11 +186,11 @@ const Field = forwardRef(
),
}}
/>
{ error && (
{error && (
<Typography
component="span"
className="input-error"
hidden={className? true: false}
hidden={className ? true : false}
Comment on lines +189 to +193
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Mom's spaghetti moment: Error visibility logic needs rethinking!

Using className to control error message visibility is an unexpected pattern that could lead to confusion. The presence of a className shouldn't determine whether errors are shown.

Consider one of these approaches:

  1. Add a dedicated prop for error visibility
  2. Remove the hidden logic entirely and let parent components control error display
- hidden={className ? true : false}
+ {/* Remove this prop and handle error visibility at the parent level */}

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Biome

[error] 193-193: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jennifer-gan I addressed this in my PR as well 👍

color={theme.palette.error.main}
mt={theme.spacing(2)}
sx={{
Expand Down
18 changes: 12 additions & 6 deletions Client/src/Pages/Infrastructure/Details/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@ import { Stack, Box, Typography } from "@mui/material";
import { useTheme } from "@emotion/react";
import CustomGauge from "../../../Components/Charts/CustomGauge";
import AreaChart from "../../../Components/Charts/AreaChart";
import { useSelector } from "react-redux";
import { networkService } from "../../../main";
import PulseDot from "../../../Components/Animated/PulseDot";
import useUtils from "../../Monitors/utils";
import { formatDurationRounded, formatDurationSplit } from "../../../Utils/timeUtils";
import axios from "axios";
import {
TzTick,
PercentTick,
Expand Down Expand Up @@ -167,6 +168,8 @@ const InfrastructureDetails = () => {
{ name: "details", path: `/infrastructure/${monitorId}` },
];
const [monitor, setMonitor] = useState(null);
const { authToken } = useSelector((state) => state.auth);
const [dateRange, setDateRange] = useState("day");
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Heads up about that dateRange state, buddy! 🤔

The dateRange is hardcoded to "day" but there's no UI component to modify it. Consider adding a date range selector to give users more control over the data visualization.

Example implementation:

const dateRangeOptions = ["day", "week", "month"];
<Select
  value={dateRange}
  onChange={(e) => setDateRange(e.target.value)}
>
  {dateRangeOptions.map(option => (
    <MenuItem key={option} value={option}>
      {option.charAt(0).toUpperCase() + option.slice(1)}
    </MenuItem>
  ))}
</Select>

const { statusColor, determineState } = useUtils();
// These calculations are needed because ResponsiveContainer
// doesn't take padding of parent/siblings into account
Expand All @@ -183,11 +186,14 @@ const InfrastructureDetails = () => {
useEffect(() => {
const fetchData = async () => {
try {
const response = await axios.get("http://localhost:5000/api/v1/dummy-data", {
headers: {
"Content-Type": "application/json",
"Cache-Control": "no-cache",
},
const response = await networkService.getStatsByMonitorId({
authToken: authToken,
monitorId: monitorId,
sortOrder: null,
limit: null,
dateRange: dateRange,
numToDisplay: 50,
normalize: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need to normalize results here, normalizing the data is used when we are displaying response time in a chart. Response time has an unbounded range so we need to normalize the data to have a reasonable visual representation.

Since we're not diplaying response time here this can safely be set to false.

});
setMonitor(response.data.data);
} catch (error) {
Expand Down