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

Feature/1484 modal to encourage starring the repo #1517

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

Feature/1484 modal to encourage starring the repo #1517

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f06c50
added a star repo modal
dikjain Dec 29, 2024
1fa3509
all set
dikjain Jan 4, 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
77 changes: 77 additions & 0 deletions Client/src/Components/Sidebar/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import ChangeLog from "../../assets/icons/changeLog.svg?react";
import Docs from "../../assets/icons/docs.svg?react";
import Folder from "../../assets/icons/folder.svg?react";
import ChatBubbleOutlineRoundedIcon from "@mui/icons-material/ChatBubbleOutlineRounded";
import CloseIcon from "@mui/icons-material/Close"; // Importing CloseIcon from MUI

import "./index.css";

Expand Down Expand Up @@ -119,6 +120,7 @@ function Sidebar() {
const [anchorEl, setAnchorEl] = useState(null);
const [popup, setPopup] = useState();
const { user } = useSelector((state) => state.auth);
const [showStarRepo, setShowStarRepo] = useState(true);

const accountMenuItem = menu.find((item) => item.name === "Account");
if (user.role?.includes("demo") && accountMenuItem) {
Expand Down Expand Up @@ -156,6 +158,18 @@ function Sidebar() {
}
}, [location]);

useEffect(() => {
const hasClosed = localStorage.getItem("starRepoClosed");
Copy link
Collaborator

@ajhollid ajhollid Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is an insufficient solution for whether or not a user has closed the star repo modal. If the user uses a different device/browser or clears their browser data the repo will show again.

A complete solution would require a server side implementation, ie storing a flag in the User model in the database, and persisting that state in the Redux store on the front end with the rest of the user data.

update

For the purposes of this PR we can accept this solution, please add a TODO comment noting that the solution is incomplete and will eventually need to be addressed. Thank you!

if (hasClosed) {
setShowStarRepo(false);
}
}, []);

const handleCloseStarRepo = () => {
setShowStarRepo(false);
localStorage.setItem("starRepoClosed", "true");
};

/* TODO refactor this, there are a some ternaries and comments in the return */

return (
Expand Down Expand Up @@ -508,6 +522,69 @@ function Sidebar() {
</List>
<Divider sx={{ mt: "auto" }} />

{showStarRepo && (
<Box
sx={{
backgroundColor: theme.palette.background.paper,
padding: theme.spacing(4),
borderRadius: theme.shape.borderRadius,
position: "relative",
height: theme.spacing(50), // Converted 100px to theme spacing
Copy link
Collaborator

Choose a reason for hiding this comment

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

Spacing seems an semantically incorrect value to use for height. It would also probably be ideal to use a relative height here as well.

display: "flex",
flexDirection: "column",
justifyContent: "center",
margin: theme.spacing(3),
}}
>
<Typography
sx={{
fontWeight: theme.typography.fontWeightBold, // Use theme for font weight
marginTop: theme.spacing(1),
color: theme.palette.text.primary,
fontSize: theme.typography.pxToRem(16) // Use theme for font size
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 some reason not to use the defined font sizes?

const typographyLevels = {
	base: typographyBase,
	xs: `${(typographyBase - 4) / 16}rem`,
	s: `${(typographyBase - 2) / 16}rem`,
	m: `${typographyBase / 16}rem`,
	l: `${(typographyBase + 2) / 16}rem`,
	xl: `${(typographyBase + 10) / 16}rem`,
};

Why are we overriding the theme typography levels? This appears to be an anti-pattern to me, to first have sizes set in the theme and then override them?

}}
variant="body2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are declaring a variant to use then we should not override that variant's styles

>
Star Checkmate
</Typography>
<Typography
sx={{
fontSize: theme.typography.pxToRem(13), // Use theme for font size
marginTop: theme.spacing(2),
color: theme.palette.text.primary
}}
variant="body2"
>
See the latest releases and help grow the community on GitHub
</Typography>
<Box
sx={{
display: "flex",
justifyContent: "start",
alignItems: "center",
marginTop: theme.spacing(2.5), // Converted 10px to theme spacing
marginBottom: theme.spacing(2.5) // Converted 10px to theme spacing
}}
>
<a href="https://github.com/bluewave-labs/checkmate" target="_blank" rel="noopener noreferrer">
<img src="https://img.shields.io/github/stars/bluewave-labs/checkmate?label=checkmate&style=social" alt="Checkmate Logo" />
</a>
</Box>
<Box sx={{ borderTop: '1px solid', borderColor: theme.palette.divider, my: 2 }} />
<IconButton
sx={{
position: "absolute",
top: 0,
right: 0,
mt: "-10px",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why -10px? Where did this value come from?

}}
onClick={handleCloseStarRepo}
>
<CloseIcon />
</IconButton>
</Box>
)}

<Stack
direction="row"
height="50px"
Expand Down