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

Add Colored Labels for HTTP Status Codes #972

Merged
merged 7 commits into from
Dec 2, 2024
Merged

Add Colored Labels for HTTP Status Codes #972

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
560b7c4
Add Http Status Label to Incident Table
rog404 Oct 21, 2024
6079449
Merge branch 'develop' into rog404/http-status-label
rog404 Oct 21, 2024
1347ccd
Prettier Adjustment
rog404 Oct 21, 2024
95a0ad0
Refact: create a new HttpStatusLabel component
rog404 Oct 25, 2024
d51ff3a
Fix some files typo
rog404 Oct 25, 2024
fd88df5
Merge branch 'develop' into rog404/http-status-label
rog404 Oct 25, 2024
6d31c94
Refactor: manage statusCode handling within the child component
rog404 Nov 18, 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
65 changes: 65 additions & 0 deletions Client/src/Components/HttpStatusLabel/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import PropTypes from "prop-types";
import { useTheme } from "@mui/material";
import { BaseLabel } from "../Label";

/**
* @component
* @param {Object} props
* @param {number | 'N/A'} props.status - The http status for the label
* @param {Styles} props.customStyles - CSS Styles passed from parent component
* @returns {JSX.Element}
* @example
* // Render a http status label
* <HttpStatusLabel status="404" />
*/
rog404 marked this conversation as resolved.
Show resolved Hide resolved

const getRoundedStatusCode = (status) => {
if (typeof status === "number" && status >= 100 && status < 600) {
return Math.floor(status / 100) * 100;
}
return status;
};
rog404 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment continues the one made in IncidentTable. We are going to deal with different scenarios here. if we don't have a number between 100 and 600, we are going to return "default" (I suggest changing N/A to default).

Copy link
Collaborator

@ajhollid ajhollid Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As an addendum to this comment, I would say that type checking here isn't necessary, we should guarantee that this function only receives a number upstream anyways.

This function also has a mixed return type which is dangerous. It will either return a number or undefined.

JavaScript's type coercion allows us to get away with this, but now we must always do a check or null coalescing whenever we use this function upstream to see if we have a value or not.

As @marcelluscaio mentioned we can avoid this by returning a default value.

if(status >= 100 && status < 600){
   // return roudned status code
}
return defaultCode

I'd say that is a safer and more reliable function. Otherwise we must always do

if ( getRoundedStatusCode(status) !== undefined ) {
   // do stuff here
}

or at the very least use the logical or to coalesce a value:

const roundedStatusCode = getRoundedStatusCode(status) || 5000

whenever we use the function. This adds extra verbosity and complexity to the code which is easily avoided.

One of the reasons I'm an advocate for strongly typed languages.. or TypeScript at least 😂


const HttpStatusLabel = ({ status, customStyles }) => {
const theme = useTheme();
const colors = {
400: {
dotColor: theme.palette.warning.main,
bgColor: theme.palette.warning.bg,
borderColor: theme.palette.warning.light,
},
500: {
dotColor: theme.palette.error.main,
bgColor: theme.palette.error.bg,
borderColor: theme.palette.error.light,
},
"N/A": {
dotColor: theme.palette.unresolved.main,
bgColor: theme.palette.unresolved.bg,
borderColor: theme.palette.unresolved.light,
},
};

const { borderColor, bgColor, dotColor } =
colors[getRoundedStatusCode(status)] || colors["N/A"];
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can avoid the nullish coalescing here by slightly refacotring the getRoudnedStatusCode funcition mentioned above


return (
<BaseLabel
label={String(status)}
styles={{
color: dotColor,
backgroundColor: bgColor,
borderColor: borderColor,
...customStyles,
}}
/>
);
};

// TODO: Restrict status type to accept only numeric values after "N/A" status in IncidentTable is resolved
HttpStatusLabel.propTypes = {
status: PropTypes.oneOfType([PropTypes.number, PropTypes.oneOf(["N/A"])]).isRequired,
customStyles: PropTypes.object,
};

export { HttpStatusLabel };
14 changes: 12 additions & 2 deletions Client/src/Components/Label/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,16 @@ const StatusLabel = ({ status, text, customStyles }) => {
bgColor: theme.palette.warning.bg,
borderColor: theme.palette.warning.light,
},
400: {
dotColor: theme.palette.warning.main,
bgColor: theme.palette.warning.bg,
borderColor: theme.palette.warning.light,
},
500: {
dotColor: theme.palette.warning.main,
bgColor: theme.palette.warning.bg,
borderColor: theme.palette.warning.light,
},
rog404 marked this conversation as resolved.
Show resolved Hide resolved
rog404 marked this conversation as resolved.
Show resolved Hide resolved
"cannot resolve": {
dotColor: theme.palette.unresolved.main,
bgColor: theme.palette.unresolved.bg,
Expand All @@ -153,7 +163,7 @@ const StatusLabel = ({ status, text, customStyles }) => {
};

// Look up the color for the status
const { borderColor, bgColor, dotColor } = colors[status];
const { borderColor, bgColor, dotColor } = colors[status] || colors["cannot resolve"];

return (
<BaseLabel
Expand Down Expand Up @@ -182,4 +192,4 @@ StatusLabel.propTypes = {
customStyles: PropTypes.object,
};

export { ColoredLabel, StatusLabel };
export { BaseLabel, ColoredLabel, StatusLabel };
7 changes: 5 additions & 2 deletions Client/src/Pages/Incidents/IncidentTable/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { useTheme } from "@emotion/react";
import { formatDateWithTz } from "../../../Utils/timeUtils";
import PlaceholderLight from "../../../assets/Images/data_placeholder.svg?react";
import PlaceholderDark from "../../../assets/Images/data_placeholder_dark.svg?react";
import { HttpStatusLabel } from "../../../Components/HttpStatusLabel";

const IncidentTable = ({ monitors, selectedMonitor, filter }) => {
const uiTimezone = useSelector((state) => state.ui.timezone);
Expand Down Expand Up @@ -182,7 +183,7 @@ const IncidentTable = ({ monitors, selectedMonitor, filter }) => {
"YYYY-MM-DD HH:mm:ss A",
uiTimezone
);

return (
<TableRow key={check._id}>
<TableCell>{monitors[check.monitorId]?.name}</TableCell>
Expand All @@ -194,7 +195,9 @@ const IncidentTable = ({ monitors, selectedMonitor, filter }) => {
/>
</TableCell>
<TableCell>{formattedDate}</TableCell>
<TableCell>{check.statusCode ? check.statusCode : "N/A"}</TableCell>
<TableCell>
<HttpStatusLabel status={check.statusCode || "N/A"} />
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation is great. Something I was thinking, what if status code is somthing other than undefined, and it isn't a number between 100 and 600? We sould be sending an uncovered scenario to the HttpStatusLabel component.

What I suggest is we just pass down chueck.statusCode, and inside of http status label we deal with that

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, let's handle the logic inside of HttpStatusLabel since we're dealing with it there for colors anyway

</TableCell>
<TableCell>{check.message}</TableCell>
</TableRow>
);
Expand Down