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

Fix: No Incidents Message #1763

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
+93 −0
Open

Fix: No Incidents Message #1763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
82 changes: 82 additions & 0 deletions src/Components/Charts/ChartBox/EmptyView.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// Components
import { Typography, Stack } from "@mui/material";
import PropTypes from "prop-types";
import { useTheme } from "@emotion/react";
import IconBox from "../../IconBox";

const EmptyView = ({
icon,
header,
message = "No Data",
size = "h2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

h2 is not a size, this prop is misleading.

h2 is a heading level

borderRadiusRight = 4,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need border radius as a prop? This seems excessive for configuration

justifyContent = "flex-start",
height = "300px"
Copy link
Collaborator

Choose a reason for hiding this comment

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

No hardcoded values please, especially non relative dimensions

}) => {
const theme = useTheme();
return (
<Stack
flex={1}
direction="row"
sx={{
backgroundColor: theme.palette.primary.main,

border: 1,
borderStyle: "solid",
borderColor: theme.palette.primary.lowContrast,
borderRadius: 2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most borders in the application are radius of 4, any reason for 2 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following the styling defined in chartbox low level component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2025-02-23 at 12 15 51 AM

Using borderRadius as 4 makes the UI inconsistent for chartboxes.
BorderRadius: 2 is implemented similar to ChartBox Component.

borderTopRightRadius: borderRadiusRight,
borderBottomRightRadius: borderRadiusRight,
}}
>
<Stack
flex={1}
alignItems="center"
sx={{
padding: theme.spacing(8),
justifyContent,
gap: theme.spacing(8),
height,
minWidth: 250,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hardcoded value

"& h2": {
color: theme.palette.primary.contrastTextSecondary,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't override heading text colors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2025-02-23 at 12 08 57 AM

The heading and text theme styles were overridden to match the chart box component similar to how they were implemented in that component., ensuring a consistent UI flow. Without this override, the Empty View would appear inconsistent.

fontSize: 15,
fontWeight: 500,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do not override heading font sizes or weights

},

"& tspan, & text": {
fill: theme.palette.primary.contrastTextTertiary,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The theme styles fonts and they should not be overridden unless there is a very good reason

},
}}
>
<Stack
alignSelf="flex-start"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason for alignSelf in a child of a flex box component?

You should be able to align this component using the parent flex component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this specific case, using alignSelf: "flex-start" is intentional and necessary to achieve the desired layout similar to how it was implement in chartbox component.
The goal is to align the header and icon to the start of the flex container while keeping the rest of the content centered.

Without the alignSelf property. The UI looks as such.
Screenshot 2025-02-23 at 12 37 25 AM

direction="row"
alignItems="center"
gap={theme.spacing(6)}
>
{icon && <IconBox>{icon}</IconBox>}
{header && <Typography component="h2">{header}</Typography>}
</Stack>
<Stack
flex={1}
justifyContent="center"
alignItems="center"
>
<Typography component={size}>
{message}
</Typography>
</Stack>
</Stack>
</Stack>
);
};

EmptyView.propTypes = {
message: PropTypes.string,
icon: PropTypes.node,
header: PropTypes.string,
size: PropTypes.oneOf(['h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'body1', 'body2'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't believe we provide styling for anything beyond h3, please remove anything beyond h3

};
Comment on lines +75 to +80
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 time! Let's fix those PropTypes! 🍝

Static analysis caught some missing PropTypes. Let's add them to keep our code robust!

 EmptyView.propTypes = {
     message: PropTypes.string,
     icon: PropTypes.node,
     header: PropTypes.string,
-    size: PropTypes.oneOf(['h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'body1', 'body2'])
+    size: PropTypes.oneOf(['h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'body1', 'body2']),
+    borderRadiusRight: PropTypes.number,
+    justifyContent: PropTypes.string,
+    height: PropTypes.string
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
EmptyView.propTypes = {
message: PropTypes.string,
icon: PropTypes.node,
header: PropTypes.string,
size: PropTypes.oneOf(['h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'body1', 'body2'])
};
EmptyView.propTypes = {
message: PropTypes.string,
icon: PropTypes.node,
header: PropTypes.string,
size: PropTypes.oneOf(['h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'body1', 'body2']),
borderRadiusRight: PropTypes.number,
justifyContent: PropTypes.string,
height: PropTypes.string
};


export default EmptyView;
8 changes: 8 additions & 0 deletions src/Components/Charts/ChartBox/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Stack, Typography } from "@mui/material";
import { useTheme } from "@emotion/react";
import IconBox from "../../IconBox";
import EmptyView from "./EmptyView";
import PropTypes from "prop-types";

const ChartBox = ({
Expand All @@ -12,8 +13,13 @@ const ChartBox = ({
Legend,
borderRadiusRight = 4,
sx,
noDataMessage,
isEmpty = false,
}) => {
const theme = useTheme();
if (isEmpty) {
return <EmptyView icon={icon} header={header} message={noDataMessage} />;
}
return (
<Stack
flex={1}
Expand Down Expand Up @@ -88,4 +94,6 @@ ChartBox.propTypes = {
icon: PropTypes.node,
header: PropTypes.string,
height: PropTypes.string,
noDataMessage: PropTypes.string,
isEmpty: PropTypes.bool
};
3 changes: 3 additions & 0 deletions src/Pages/Uptime/Details/Components/ChartBoxes/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ const ChartBoxes = ({
<ChartBox
icon={<UptimeIcon />}
header="Uptime"
isEmpty={monitor?.uptimePercentage === 0 && !monitor?.upChecks?.length}
>
<Stack
width={"100%"}
Expand Down Expand Up @@ -95,6 +96,8 @@ const ChartBoxes = ({
<ChartBox
icon={<IncidentsIcon />}
header="Incidents"
noDataMessage="Great. No Incidents, yet!"
isEmpty={!monitor?.groupedDownChecks?.length}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please avoid truthy falsey comparisons, use strict evaluations so it's clear what you are trying to do

>
<Stack width={"100%"}>
<Box position="relative">
Expand Down