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

FIXED: Sidebar design changes #1525

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

FIXED: Sidebar design changes #1525

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8cd61cd
changes the sidebar
ahmadnadeem6236 Jan 7, 2025
41bc521
refactor the code
ahmadnadeem6236 Jan 7, 2025
fe5802c
fix the spacing
ahmadnadeem6236 Jan 7, 2025
31d9e13
deleted the commented code
ahmadnadeem6236 Jan 8, 2025
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
340 changes: 314 additions & 26 deletions Client/src/Components/Sidebar/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,34 +55,38 @@ const menu = [
// { name: "Status pages", path: "status", icon: <StatusPages /> },
{ name: "Maintenance", path: "maintenance", icon: <Maintenance /> },
// { name: "Integrations", path: "integrations", icon: <Integrations /> },
{
name: "Account",
icon: <Account />,
nested: [
{ name: "Profile", path: "account/profile", icon: <UserSvg /> },
{ name: "Password", path: "account/password", icon: <LockSvg /> },
{ name: "Team", path: "account/team", icon: <TeamSvg /> },
],
},
//{
// name: "Account",
// icon: <Account />,
// nested: [
// { name: "Profile", path: "account/profile", icon: <UserSvg /> },
// { name: "Password", path: "account/password", icon: <LockSvg /> },
// { name: "Team", path: "account/team", icon: <TeamSvg /> },
// ],
//},
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Yo! Remove the commented-out menu sections.

Since these menu items have been intentionally moved as per the PR objectives, we should remove the commented code instead of leaving it in the file. Version control (git) already maintains the history if we need to reference it later.

-	//{
-	//	name: "Account",
-	//	icon: <Account />,
-	//	nested: [
-	//		{ name: "Profile", path: "account/profile", icon: <UserSvg /> },
-	//		{ name: "Password", path: "account/password", icon: <LockSvg /> },
-	//		{ name: "Team", path: "account/team", icon: <TeamSvg /> },
-	//	],
-	//},

-	// {
-	// 	name: "Other",
-	// 	icon: <Folder />,
-	// 	nested: [
-	// 	],
-	// },

Also applies to: 73-78

{
name: "Settings",
icon: <Settings />,
path: "settings",
},

// {
// name: "Other",
// icon: <Folder />,
// nested: [
// ],
// },
];

const subMenu = [
{ name: "Support", path: "support", icon: <Support /> },
{
name: "Other",
icon: <Folder />,
nested: [
{ name: "Support", path: "support", icon: <Support /> },
{
name: "Discussions",
path: "discussions",
icon: <ChatBubbleOutlineRoundedIcon />,
},
{ name: "Docs", path: "docs", icon: <Docs /> },
{ name: "Changelog", path: "changelog", icon: <ChangeLog /> },
],
name: "Discussions",
path: "discussions",
icon: <ChatBubbleOutlineRoundedIcon />,
},
{ name: "Docs", path: "docs", icon: <Docs /> },
{ name: "Changelog", path: "changelog", icon: <ChangeLog /> },
Comment on lines +65 to +73
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Yo! Found some spaghetti in the URL situation! 🍝

The subMenu paths (support, discussions, docs, changelog) don't have corresponding URL mappings in the codebase. This could lead to broken navigation. Found some URLs in docs, but they don't match the sidebar structure:

  • Support points to Discord
  • Discussions has two different GitHub URLs
  • Docs points to an API docs page
  • No changelog URL found anywhere
🔗 Analysis chain

Yo! Verify external URLs in the subMenu array.

The implementation looks clean, but we should verify that the URLs in URL_MAP match the paths defined in subMenu.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all subMenu paths have corresponding URLs in URL_MAP
echo "Checking URL_MAP entries for subMenu paths..."
for path in "support" "discussions" "docs" "changelog"; do
  rg -q "\"$path\":.+\"https://" && echo "✓ Found URL for $path" || echo "⨯ Missing URL for $path"
done

Length of output: 709

Script:

#!/bin/bash
# Search for any URL mapping or configuration
echo "=== Searching for URL mapping patterns ==="
rg -i "const.*map.*=.*{" -A 5
echo -e "\n=== Searching for URL/route configurations ==="
rg "(url|route).*map" -A 5
echo -e "\n=== Searching for these specific paths in any context ==="
rg -l "(support|discussions|docs|changelog).*https?://"

Length of output: 2431

Script:

#!/bin/bash
echo "=== Checking Sidebar component for URL definitions ==="
rg -A 5 "(const|let|var).*(URL|url).*=.*{" Client/src/Components/Sidebar/index.jsx

echo -e "\n=== Checking documentation for URL patterns ==="
for file in README.md guides/users-guide/quickstart.md guides/README.md; do
  echo -e "\nFile: $file"
  rg "(support|discussions|docs|changelog).*https?://" "$file" || true
done

Length of output: 1669

];

/* TODO this could be a key in nested Path would be the link */
Expand Down Expand Up @@ -270,11 +274,253 @@ function Sidebar() {
}
sx={{
px: theme.spacing(6),
height: "30%",
}}
>
{menu.map((item) =>
item.path ? (
<Tooltip
key={item.path}
placement="right"
title={collapsed ? item.name : ""}
slotProps={{
popper: {
modifiers: [
{
name: "offset",
options: {
offset: [0, -16],
},
},
],
},
}}
disableInteractive
>
<ListItemButton
className={location.pathname.includes(item.path) ? "selected-path" : ""}
onClick={() => navigate(`/${item.path}`)}
sx={{
height: "37px",
gap: theme.spacing(4),
borderRadius: theme.shape.borderRadius,
px: theme.spacing(4),
}}
>
<ListItemIcon sx={{ minWidth: 0 }}>{item.icon}</ListItemIcon>
<ListItemText>{item.name}</ListItemText>
</ListItemButton>
</Tooltip>
) : collapsed ? (
/* TODO Do we ever get here? */
<React.Fragment key={item.name}>
<Tooltip
placement="right"
title={collapsed ? item.name : ""}
slotProps={{
popper: {
modifiers: [
{
name: "offset",
options: {
offset: [0, -16],
},
},
],
},
}}
disableInteractive
>
<ListItemButton
className={
Boolean(anchorEl) && popup === item.name ? "selected-path" : ""
}
onClick={(event) => openPopup(event, item.name)}
sx={{
position: "relative",
gap: theme.spacing(4),
borderRadius: theme.shape.borderRadius,
px: theme.spacing(4),
}}
>
<ListItemIcon sx={{ minWidth: 0 }}>{item.icon}</ListItemIcon>
<ListItemText>{item.name}</ListItemText>
</ListItemButton>
</Tooltip>
{/* <Menu
className="sidebar-popup"
anchorEl={anchorEl}
open={Boolean(anchorEl) && popup === item.name}
onClose={closePopup}
disableScrollLock
anchorOrigin={{
vertical: "top",
horizontal: "right",
}}
slotProps={{
paper: {
sx: {
mt: theme.spacing(-2),
ml: theme.spacing(1),
},
},
}}
MenuListProps={{ sx: { px: 1, py: 2 } }}
sx={{
ml: theme.spacing(8),
"& .selected-path": {
backgroundColor: theme.palette.background.accent,
},
}}
>
{item.nested.map((child) => {
if (
child.name === "Team" &&
authState.user?.role &&
!authState.user.role.includes("superadmin")
) {
return null;
}

return (
<MenuItem
className={
location.pathname.includes(child.path) ? "selected-path" : ""
}
key={child.path}
onClick={() => {
const url = URL_MAP[child.path];
if (url) {
window.open(url, "_blank", "noreferrer");
} else {
navigate(`/${child.path}`);
}
closePopup();
}}
sx={{
gap: theme.spacing(4),
opacity: 0.9,
"& svg": {
"& path": {
stroke: theme.palette.other.icon,
strokeWidth: 1.1,
},
},
}}
>
{child.icon}
{child.name}
</MenuItem>
);
})}
</Menu> */}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Knees weak! Clean up the commented Menu components.

There are large blocks of commented-out Menu component code that should be removed. This improves code readability and maintains a cleaner codebase.

Remove the commented Menu components and their associated logic since they're no longer needed after the menu restructuring.

Also applies to: 593-659

</React.Fragment>
) : (
<React.Fragment key={item.name}>
<ListItemButton
onClick={() =>
setOpen((prev) => ({
...Object.fromEntries(Object.keys(prev).map((key) => [key, false])),
[item.name]: !prev[item.name],
}))
}
sx={{
gap: theme.spacing(4),
borderRadius: theme.shape.borderRadius,
px: theme.spacing(4),
}}
>
<ListItemIcon sx={{ minWidth: 0 }}>{item.icon}</ListItemIcon>
<ListItemText>{item.name}</ListItemText>
{open[`${item.name}`] ? <ArrowUp /> : <ArrowDown />}
</ListItemButton>
<Collapse
in={open[`${item.name}`]}
timeout="auto"
>
{/* <List
component="div"
disablePadding
sx={{ pl: theme.spacing(12) }}
>
{item.nested.map((child) => {
if (
child.name === "Team" &&
authState.user?.role &&
!authState.user.role.includes("superadmin")
) {
return null;
}

return (
<ListItemButton
className={
location.pathname.includes(child.path) ? "selected-path" : ""
}
key={child.path}
onClick={() => {
const url = URL_MAP[child.path];
if (url) {
window.open(url, "_blank", "noreferrer");
} else {
navigate(`/${child.path}`);
}
}}
sx={{
gap: theme.spacing(4),
borderRadius: theme.shape.borderRadius,
pl: theme.spacing(4),
"&::before": {
content: `""`,
position: "absolute",
top: 0,
left: "-7px",
height: "100%",
borderLeft: 1,
borderLeftColor: theme.palette.other.line,
},
"&:last-child::before": {
height: "50%",
},
"&::after": {
content: `""`,
position: "absolute",
top: "45%",
left: "-8px",
height: "3px",
width: "3px",
borderRadius: "50%",
backgroundColor: theme.palette.other.line,
},
"&.selected-path::after": {
backgroundColor: theme.palette.text.tertiary,
transform: "scale(1.2)",
},
}}
>
<ListItemIcon sx={{ minWidth: 0 }}>{child.icon}</ListItemIcon>
<ListItemText>{child.name}</ListItemText>
</ListItemButton>
);
})}
</List> */}
</Collapse>
</React.Fragment>
)
)}
</List>
<List
component="nav"
aria-labelledby="nested-menu-subheader"
disablePadding
sx={{
px: theme.spacing(6),
pt: theme.spacing(60),
height: "100%",
overflow: "hidden",
}}
>
{menu.map((item) =>
{subMenu.map((item) =>
item.path ? (
<Tooltip
key={item.path}
Expand Down Expand Up @@ -344,7 +590,7 @@ function Sidebar() {
<ListItemText>{item.name}</ListItemText>
</ListItemButton>
</Tooltip>
<Menu
{/* <Menu
className="sidebar-popup"
anchorEl={anchorEl}
open={Boolean(anchorEl) && popup === item.name}
Expand Down Expand Up @@ -410,7 +656,7 @@ function Sidebar() {
</MenuItem>
);
})}
</Menu>
</Menu> */}
</React.Fragment>
) : (
<React.Fragment key={item.name}>
Expand Down Expand Up @@ -573,7 +819,7 @@ function Sidebar() {
mr: "-8px",
"&:focus": { outline: "none" },
alignSelf: "center",
padding: '10px',
padding: "10px",

"& svg": {
width: "22px",
Expand Down Expand Up @@ -639,7 +885,49 @@ function Sidebar() {
</MenuItem>
)}
{collapsed && <Divider />}
<Divider />
<MenuItem
onClick={() => navigate("account/profile")}
sx={{
gap: theme.spacing(4),
borderRadius: theme.shape.borderRadius,
pl: theme.spacing(4),
"& svg path": {
stroke: theme.palette.other.icon,
},
}}
>
<UserSvg />
Profile
</MenuItem>

<MenuItem
onClick={() => navigate("account/password")}
sx={{
gap: theme.spacing(4),
borderRadius: theme.shape.borderRadius,
pl: theme.spacing(4),
"& svg path": {
stroke: theme.palette.other.icon,
},
}}
>
<LockSvg />
Password
</MenuItem>
<MenuItem
onClick={() => navigate("account/team")}
sx={{
gap: theme.spacing(4),
borderRadius: theme.shape.borderRadius,
pl: theme.spacing(4),
"& svg path": {
stroke: theme.palette.other.icon,
},
}}
>
<TeamSvg />
Team
</MenuItem>
Comment on lines +833 to +846
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Arms are heavy! Add role check for Team menu item.

The Team menu item is missing the superadmin role check that was present in the original implementation. This could expose the Team menu item to users who shouldn't have access to it.

Add the role check:

+    {authState.user?.role?.includes("superadmin") && (
     <MenuItem
       onClick={() => navigate("account/team")}
       sx={{
         gap: theme.spacing(4),
         borderRadius: theme.shape.borderRadius,
         pl: theme.spacing(4),
         "& svg path": {
           stroke: theme.palette.other.icon,
         },
       }}
     >
       <TeamSvg />
       Team
     </MenuItem>
+    )}
📝 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
<MenuItem
onClick={() => navigate("account/team")}
sx={{
gap: theme.spacing(4),
borderRadius: theme.shape.borderRadius,
pl: theme.spacing(4),
"& svg path": {
stroke: theme.palette.other.icon,
},
}}
>
<TeamSvg />
Team
</MenuItem>
{authState.user?.role?.includes("superadmin") && (
<MenuItem
onClick={() => navigate("account/team")}
sx={{
gap: theme.spacing(4),
borderRadius: theme.shape.borderRadius,
pl: theme.spacing(4),
"& svg path": {
stroke: theme.palette.other.icon,
},
}}
>
<TeamSvg />
Team
</MenuItem>
)}

<MenuItem
onClick={logout}
sx={{
Expand Down