-
Notifications
You must be signed in to change notification settings - Fork 209
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
Feat/fe/statuspage 1 #1306
Feat/fe/statuspage 1 #1306
Changes from 3 commits
136ed04
4345d69
54c2826
6b9334b
90cfc0c
cba1d00
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
import { useState, useRef } from "react"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yo! We got some unused imports and incomplete state setup here!
- import { useState, useRef } from "react";
+ import { useState } from "react";
- import { useDispatch, useSelector } from "react-redux";
+ import { useSelector } from "react-redux";
const [localData, setLocalData] = useState({
+ isPublished: false,
+ title: "",
+ description: ""
}); Also applies to: 6-6, 11-19 |
||
import { Box, Stack, Typography } from "@mui/material"; | ||
import { ConfigBox } from "../../../Pages/Monitors/styled"; | ||
import Checkbox from "../../Inputs/Checkbox"; | ||
import { useTheme } from "@emotion/react"; | ||
import { useDispatch, useSelector } from "react-redux"; | ||
import TabPanel from "@mui/lab/TabPanel"; | ||
|
||
const GeneralSettingsPanel = () => { | ||
const theme = useTheme(); | ||
const dispatch = useDispatch(); | ||
|
||
//redux state | ||
const { user, authToken, isLoading } = useSelector((state) => state.auth); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Yo! We need to handle loading states, fam! The component imports <TabPanel
value="general settings"
sx={{
"& h1, & p, & input": {
color: theme.palette.text.tertiary,
},
+ opacity: isLoading ? 0.5 : 1,
+ pointerEvents: isLoading ? 'none' : 'auto'
}}
>
+ {isLoading && (
+ <Box sx={{ position: 'absolute', top: '50%', left: '50%', transform: 'translate(-50%, -50%)' }}>
+ <CircularProgress />
+ </Box>
+ )} Also applies to: 25-33 |
||
|
||
// Local state for form data, errors, and file handling | ||
const [localData, setLocalData] = useState({ | ||
}); | ||
const [errors, setErrors] = useState({}); | ||
const [file, setFile] = useState(); | ||
|
||
const handleSubmit = () => {}; | ||
|
||
const handleChange = () => {}; | ||
|
||
const handleBlur = () => {}; | ||
return ( | ||
<TabPanel | ||
value="general settings" | ||
sx={{ | ||
"& h1, & p, & input": { | ||
color: theme.palette.text.tertiary, | ||
}, | ||
}} | ||
> | ||
<Stack | ||
component="form" | ||
className="status-general-settings-form" | ||
onSubmit={handleSubmit} | ||
noValidate | ||
spellCheck="false" | ||
gap={theme.spacing(12)} | ||
mt={theme.spacing(6)} | ||
> | ||
<ConfigBox> | ||
<Box> | ||
<Stack gap={theme.spacing(6)}> | ||
<Typography component="h2">Access</Typography> | ||
<Typography component="p"> | ||
If your status page is ready, you can mark it as published. | ||
</Typography> | ||
</Stack> | ||
</Box> | ||
<Checkbox | ||
jennifer-gan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
id="published-to-public" | ||
label={`Published and visible to the public`} | ||
onChange={(e) => handleChange(e)} | ||
jennifer-gan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
onBlur={handleBlur} | ||
jennifer-gan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/> | ||
</ConfigBox> | ||
</Stack> | ||
</TabPanel> | ||
); | ||
}; | ||
|
||
export default GeneralSettingsPanel; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
import PropTypes from "prop-types"; | ||
import { useNavigate } from "react-router"; | ||
import { useSelector } from "react-redux"; | ||
import { Box, Tab, useTheme } from "@mui/material"; | ||
import TabContext from "@mui/lab/TabContext"; | ||
import TabList from "@mui/lab/TabList"; | ||
import GeneralSettingsPanel from "../../../Components/TabPanels/Status/GeneralSettingsPanel"; | ||
|
||
/** | ||
* CreateStatus page renders a page with tabs for general settings and contents. | ||
* @param {string} [props.open] - Specifies the initially open tab: 'general settings' or 'content'. | ||
* @returns {JSX.Element} | ||
*/ | ||
|
||
const CreateStatus = ({ open = "general settings" }) => { | ||
const theme = useTheme(); | ||
const navigate = useNavigate(); | ||
const tab = open; | ||
const handleTabChange = (event, newTab) => { | ||
navigate(`/status/${newTab}`); | ||
}; | ||
Comment on lines
+19
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we are calling this without passing the new tab. I suggest deleting the second parameter, adding a name to the input, and getting the name from the event target, then passing it to to navigate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, the onChange of TabList has the (event, value) as param/signature, so those were passed on to the handleTabChange There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shemy-gan Please avoid resolving conversations ahead of time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The document/signature of onChange has event and value, (property) onChange?: ((event: React.SyntheticEvent, value: any) => void) | undefined so here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jennifer-gan I believe the point here is that if we use the name attribute we don't have to use the second parameter. Similar to the implementation in the refactor of the various create pages I pushed the other day. Always nice to reduce the number of parameters needed if at all possible |
||
const { user } = useSelector((state) => state.auth); | ||
|
||
const requiredRoles = ["superadmin", "admin"]; | ||
let tabList = ["General Settings", "Contents"]; | ||
|
||
return ( | ||
<Box | ||
className="status" | ||
px={theme.spacing(20)} | ||
py={theme.spacing(12)} | ||
border={1} | ||
borderColor={theme.palette.border.light} | ||
borderRadius={theme.shape.borderRadius} | ||
backgroundColor={theme.palette.background.main} | ||
> | ||
<TabContext value={tab}> | ||
<Box | ||
sx={{ | ||
borderBottom: 1, | ||
borderColor: theme.palette.border.light, | ||
"& .MuiTabs-root": { height: "fit-content", minHeight: "0" }, | ||
}} | ||
> | ||
<TabList | ||
onChange={handleTabChange} | ||
aria-label="status tabs" | ||
> | ||
{tabList.map((label, index) => ( | ||
<Tab | ||
label={label} | ||
key={index} | ||
value={label.toLowerCase()} | ||
sx={{ | ||
fontSize: 13, | ||
color: theme.palette.text.tertiary, | ||
textTransform: "none", | ||
minWidth: "fit-content", | ||
minHeight: 0, | ||
paddingLeft: 0, | ||
paddingY: theme.spacing(4), | ||
fontWeight: 400, | ||
marginRight: theme.spacing(8), | ||
"&:focus": { | ||
outline: "none", | ||
}, | ||
}} | ||
/> | ||
))} | ||
</TabList> | ||
</Box> | ||
<GeneralSettingsPanel /> | ||
</TabContext> | ||
Comment on lines
+37
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion This tab context needs some error boundaries and loading states! The tab navigation could break if something goes wrong, and users won't know what's happening. Here's a more robust implementation: +const [isLoading, setIsLoading] = useState(false);
+const [error, setError] = useState(null);
+if (error) {
+ return <Box>Error loading status page: {error.message}</Box>;
+}
<TabContext value={tab}>
+ {isLoading ? (
+ <Box>Loading...</Box>
+ ) : (
<Box
sx={{
borderBottom: 1,
borderColor: theme.palette.border.light,
"& .MuiTabs-root": { height: "fit-content", minHeight: "0" },
}}
>
// ... rest of the code
</Box>
+ )}
</TabContext>
|
||
</Box> | ||
); | ||
}; | ||
|
||
CreateStatus.propTypes = { | ||
open: PropTypes.oneOf(["general settings", "contents"]), | ||
}; | ||
|
||
export default CreateStatus; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what this is doing, can you provide a print screen @shemy-gan ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case is when the label is an element instead of string, it will align the checkbox on top of the container vertically; Alex suggested to expose this explicitly, so the fix was here :54c2826
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show the two cases in a print screen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move on to feat/fe/statuspage 2, the layout is as below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Please move on to feat/fe/statuspage 2" what do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part layout has already been updated in the next branch feat/fe/statuspage 2, and there is a PR for that also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, ok. So should we close this PR, since it is already contemplated in the other one?