feat: adding infrastructure display support to status page#3343
feat: adding infrastructure display support to status page#3343akashmannil wants to merge 1 commit intodevelopfrom
Conversation
akashmannil
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
ajhollid
left a comment
ajhollid
left a comment
There was a problem hiding this comment.
In general, it seems to work well.
My main concern is the abundance of complexity and conditional rendering.
I think we need to spend some time simplifying this, extracting complex rendering logic, and cleaning up as much as possible.
Aside from that, we need to be extremely careful with the localization file. If we have "http" declared for a type of monitor somewhere else ("optionHttp": "HTTP(S)") , we don't want to do it again for the badges.
If we do, we now have to make sure these always stay in sync, which is frustrating. If the same string needs to be used in multiple places it should be extracted to the "common" key in en.json.
| "docker": "Docker", | ||
| "pagespeed": "PageSpeed", | ||
| "hardware": "Infrastructure", | ||
| "game": "Game" |
There was a problem hiding this comment.
I have a feeling these values are all declared somewhere else already, can you please thoroughly check the localization file? Duplicated strings is a maintenance nightmare
| </Typography> | ||
| <Typography variant="body2">{upperValue}</Typography> | ||
| </Stack> | ||
| )} |
There was a problem hiding this comment.
I believe we can better handle this nested conditional rendering.
The rendering condition for the inner stack is equivalent to
if (A or B){
if (A) {
// do A
}
if (B) {
// do B
}
}
At this level of complexity, it's probably time to define a component that internalizes this logic in a clear easy to understand way
| paddingBottom={theme.spacing(10)} | ||
| > | ||
| {upperLabel && ( | ||
| <Stack |
There was a problem hiding this comment.
This is quite deeply nested. We can probably flatten this layout considerably using grids
| > | ||
| <Typography | ||
| variant="body2" | ||
| color="text.secondary" |
There was a problem hiding this comment.
We generally don't use this contention in the project, let's stick with referencing the palette for theme colors for consistency.
|
|
||
| const disks = latestCheck?.disk ?? []; | ||
| const totalDiskUsage = disks.reduce((acc, disk) => acc + (disk?.usage_percent || 0), 0); | ||
| const diskCount = disks.length || 1; |
There was a problem hiding this comment.
This seems problematic, if disks is empty, ie no disks, this will evaluate to 1 🤔
If there are no discs, there should be no disk metrics and we should return.
| {!isInfrastructureMonitor && ( | ||
| <> | ||
| {statusPage.showCharts !== false && ( | ||
| <Box | ||
| sx={{ | ||
| overflow: "hidden", | ||
| minWidth: 0, | ||
| flex: 1, | ||
| mb: theme.spacing(2), | ||
| }} | ||
| > | ||
| {chartType === "histogram" ? ( | ||
| <HistogramResponseTime | ||
| height={{ xs: 50, md: 100 }} | ||
| gap={{ xs: theme.spacing(0.5), md: theme.spacing(5) }} | ||
| checks={monitor?.checks?.slice().reverse() ?? []} | ||
| /> | ||
| ) : ( | ||
| <HeatmapResponseTime | ||
| checks={monitor?.checks?.slice().reverse() ?? []} | ||
| /> | ||
| )} | ||
| </Box> | ||
| )} | ||
| </Box> | ||
| </> | ||
| )} | ||
|
|
||
| {isInfrastructureMonitor && statusPage.showInfrastructure !== false && ( | ||
| <BaseBox | ||
| key={monitor.id} | ||
| padding={theme.spacing(4)} | ||
| > | ||
| <Stack | ||
| direction="row" | ||
| alignItems="center" | ||
| justifyContent="space-between" | ||
| gap={theme.spacing(8)} | ||
| ></Stack> | ||
| <InfrastructureMetrics monitor={monitor} /> | ||
| </BaseBox> |
There was a problem hiding this comment.
This rendering logic is also becoming deeply nested, probably time to extract here as well
| monitor?.type === "hardware" | ||
| ? theme.palette.info.light | ||
| : theme.palette.success.light, | ||
| color: | ||
| monitor?.type === "hardware" | ||
| ? theme.palette.info.dark | ||
| : theme.palette.success.dark, |
There was a problem hiding this comment.
Let's create helper functions for these, there may be more monitor types in the future and having this all inline ternaries will become messy fast.
Additionally, they are somewhat difficult to read 🤔 I don't think there's enough contrast between the bg and the text. Probably a black/transparent bg with colored text would be more readable.
| monitor?.type === "hardware" | ||
| ? theme.palette.info.dark | ||
| : theme.palette.success.dark, | ||
| padding: `${theme.spacing(0.5)} ${theme.spacing(1.5)}`, |
There was a problem hiding this comment.
Why fractional spacings?
| {isInfrastructureMonitor && statusPage.showInfrastructure !== false && ( | ||
| <BaseBox | ||
| key={monitor.id} | ||
| padding={theme.spacing(4)} | ||
| > | ||
| <Stack | ||
| direction="row" | ||
| alignItems="center" | ||
| justifyContent="space-between" | ||
| gap={theme.spacing(8)} | ||
| ></Stack> | ||
| <InfrastructureMetrics monitor={monitor} /> |
There was a problem hiding this comment.
What's going on here actually? Why do we have a BaseBox inside another BaseBox, holding an empty stack?
A BaseBox should be a base component 🤔 If showing infra stats is set to false, lets just display the regular histogram then rather than an empty row.
2 participants
These changes focus on adding Infrastructure section to currently existing status Page.
Fixes #3305
Status.-.Infra.sections.mp4
Latest UI after adding a little bit more padding.
<div>Add</div>, use):npm run formatin server and client directories, which automatically formats your code.