Fix/379 button design in the home page #396
Conversation
WalkthroughThis pull request introduces several UI-related enhancements, focusing on skeleton components and button styling in the frontend. The changes include removing an existing button styles file, updating the Changes
Sequence DiagramsequenceDiagram
participant Dashboard
participant CreateActivityButton
participant BannerSkeleton
participant CustomSkeleton
Dashboard->>CreateActivityButton: Render with skeletonType
alt Banner Creation
CreateActivityButton-->>BannerSkeleton: Use as loading state
else Helper Link
CreateActivityButton-->>CustomSkeleton: Use as loading state
end
Possibly Related PRs
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
frontend/src/components/HomePageComponents/CreateActivityButton/CreateActivityButton.jsx (2)
25-29: Mom's spaghetti in this transition! 🍝The transition property specifies
box-shadowbut no box-shadow is defined in the base or hover states.transition: "box-shadow 0.3s ease", "&:hover": { border: "1px solid var(--light-border-color)", backgroundColor: "var(--light-gray)", + boxShadow: "0 2px 4px rgba(0, 0, 0, 0.1)", },
39-41: Props validation weak like my knees! 🍝Consider adding more specific PropTypes validation:
-skeletonType: PropTypes.node, -placeholder: PropTypes.string, -onClick: PropTypes.func.isRequired, +skeletonType: PropTypes.node, +placeholder: PropTypes.string.isRequired, +onClick: PropTypes.func.isRequired,frontend/src/components/HomePageComponents/Skeletons/PopUpSkeleton.jsx (2)
36-47: These hardcoded values got my arms heavy! 🍝Consider moving the magic numbers and colors to a theme or constants file:
+const SKELETON_CONSTANTS = { + POPUP: { + width: 100, + height: 50, + top: 20, + left: 30, + }, +}; <Skeleton sx={{ position: "absolute", - top: 20, - left: 30, + top: SKELETON_CONSTANTS.POPUP.top, + left: SKELETON_CONSTANTS.POPUP.left, bgcolor: "var(--light-purple)", border: "0.5px solid #e9d5ff", }} variant="rounded" - width={100} - height={50} + width={SKELETON_CONSTANTS.POPUP.width} + height={SKELETON_CONSTANTS.POPUP.height} animation={false} />
1-52: Why's the animation disabled? Lost yourself in the moment! 🍝Consider enabling animation for better loading state feedback. The current implementation might appear static and unresponsive to users.
Remove all
animation={false}props to enable the default wave animation, or explicitly setanimation="wave"for a specific animation style.frontend/src/scenes/dashboard/Dashboard.jsx (3)
8-10: These imports are looking fresh!Clean organization of skeleton imports. Consider creating an index file to barrel these exports.
Create an index.js in the Skeletons folder:
export { default as PopUpSkeleton } from './PopUpSkeleton'; export { default as BannerSkeleton } from './BannerSkeleton'; export { default as HelperSkeleton } from './HelperSkeleton';Then simplify your imports:
- import PopUpSkeleton from "../../components/HomePageComponents/Skeletons/PopUpSkeleton"; - import BannerSkeleton from "../../components/HomePageComponents/Skeletons/BannerSkeleton"; - import HelperSkeleton from "../../components/HomePageComponents/Skeletons/HelperSkeleton"; + import { PopUpSkeleton, BannerSkeleton, HelperSkeleton } from "../../components/HomePageComponents/Skeletons";
Line range hint
14-19: Yo, these metrics should be in their own file!The metrics array could be moved to a constants file to keep the component cleaner.
Create a new file
constants/metrics.js:export const DASHBOARD_METRICS = [ { metricName: "Popup views", metricValue: 5000, changeRate: 5 }, { metricName: "Hint views", metricValue: 2000, changeRate: -20 }, { metricName: "Banner Views", metricValue: 3000, changeRate: 15 }, ];
21-35: The buttons array is looking clean, but we can make it even better!Good job adding the skeleton types consistently. Consider extracting this configuration to a separate file and adding TypeScript interfaces for better type safety.
Create types and constants files:
// types/buttons.ts interface DashboardButton { skeletonType: React.ReactNode; placeholder: string; onClick: () => void; } // constants/buttons.ts export const DASHBOARD_BUTTONS = (navigate: NavigateFunction): DashboardButton[] => [ { skeletonType: <PopUpSkeleton />, placeholder: "Create a popup", onClick: () => navigate("/popup/create"), }, // ... other buttons ];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/src/components/HomePageComponents/CreateActivityButton/ActivityButtonStyles.js(0 hunks)frontend/src/components/HomePageComponents/CreateActivityButton/CreateActivityButton.jsx(1 hunks)frontend/src/components/HomePageComponents/CreateActivityButtonList/CreateActivityButtonList.jsx(1 hunks)frontend/src/components/HomePageComponents/Skeletons/BannerSkeleton.jsx(1 hunks)frontend/src/components/HomePageComponents/Skeletons/HelperSkeleton.jsx(1 hunks)frontend/src/components/HomePageComponents/Skeletons/PopUpSkeleton.jsx(1 hunks)frontend/src/components/HomePageComponents/Skeletons/Skeleton.module.scss(1 hunks)frontend/src/scenes/dashboard/Dashboard.jsx(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/components/HomePageComponents/CreateActivityButton/ActivityButtonStyles.js
✅ Files skipped from review due to trivial changes (2)
- frontend/src/components/HomePageComponents/Skeletons/Skeleton.module.scss
- frontend/src/components/HomePageComponents/Skeletons/BannerSkeleton.jsx
🔇 Additional comments (3)
frontend/src/components/HomePageComponents/CreateActivityButtonList/CreateActivityButtonList.jsx (1)
8-10: 🛠️ Refactor suggestion
Yo dawg, we need a better key for these buttons! 🍝
Using array index as key might cause issues if buttons get reordered or filtered. Consider using a unique identifier from the button object instead.
-{buttons.map((button, index) => (
- <CreateActivityButton key={index} {...button} />
-))}
+{buttons.map((button) => (
+ <CreateActivityButton key={button.id} {...button} />
-))}frontend/src/components/HomePageComponents/CreateActivityButton/CreateActivityButton.jsx (1)
16-19: Yo these CSS variables making me nervous! 🍝
Let's verify that all CSS variables are defined in the theme:
- --background-color
- --grey-border
- --main-text-color
✅ Verification successful
Yo dawg, those CSS variables are legit! All good in the hood! 🍝
All the CSS variables used in the component are properly defined in frontend/src/styles/variables.css:
--background-color✅--grey-border✅--main-text-color✅
Plus they're getting mad love across the codebase, being used in like 30+ other files. We're keeping it real with consistent theming!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CSS variable definitions
rg -g '*.{css,scss}' '(--background-color|--grey-border|--main-text-color)'
Length of output: 3902
frontend/src/components/HomePageComponents/Skeletons/HelperSkeleton.jsx (1)
36-47: Watch out for that absolute positioning, homie!
That absolute positioning at the bottom right could cause layout issues if the parent container's size changes. Consider using CSS Grid or Flexbox for more reliable positioning.
Let's check if this component is used in different container sizes:
✅ Verification successful
Yo, that absolute positioning is actually solid here, fam!
The absolute positioning is safe in this case because:
- The parent
.skeletonContainerhasposition: relative - The component is used in a consistent way within the Dashboard's
CreateActivityButtonList - The container has fixed padding and a defined structure with flex layout
The current implementation provides a reliable and consistent layout for the skeleton component within its container. No changes needed, it's already following best practices!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
rg -l "HelperSkeleton" --type jsx --type tsx
Length of output: 72
Script:
#!/bin/bash
# Let's try a different approach to find the component usage
rg -l "HelperSkeleton" --type-list # First, check available file types
rg -l "HelperSkeleton" # Then search without type restriction
Length of output: 563
Script:
#!/bin/bash
# Let's also check the parent component structure
fd HelperSkeleton.jsx
cat $(fd HelperSkeleton.jsx)
Length of output: 1404
Script:
#!/bin/bash
# Let's look for imports of this component
rg "import.*HelperSkeleton"
Length of output: 164
Script:
#!/bin/bash
# Let's check how HelperSkeleton is used in Dashboard.jsx
cat frontend/src/scenes/dashboard/Dashboard.jsx
# Let's also check the Skeleton.module.scss to see the container styles
cat frontend/src/components/HomePageComponents/Skeletons/Skeleton.module.scss
Length of output: 2432
| <Skeleton | ||
| variant="rectangular" | ||
| width={210} | ||
| height={30} | ||
| sx={{ bgcolor: "var(--light-gray)" }} | ||
| animation={false} | ||
| /> | ||
| <Skeleton | ||
| variant="rectangular" | ||
| width={210} | ||
| height={30} | ||
| sx={{ bgcolor: "var(--light-gray)" }} | ||
| animation={false} | ||
| /> | ||
| <Skeleton | ||
| variant="rectangular" | ||
| width={210} | ||
| height={30} | ||
| sx={{ bgcolor: "var(--light-gray)" }} | ||
| animation={false} | ||
| /> | ||
| <Skeleton | ||
| variant="rectangular" | ||
| width={210} | ||
| height={30} | ||
| sx={{ bgcolor: "var(--light-gray)" }} | ||
| animation={false} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
There's vomit on his sweater already - it's repeated code! 🍝
Let's clean up this repetition by extracting the common Skeleton configuration:
+const CommonSkeleton = () => (
+ <Skeleton
+ variant="rectangular"
+ width={210}
+ height={30}
+ sx={{ bgcolor: "var(--light-gray)" }}
+ animation={false}
+ />
+);
return (
<div className={styles.skeletonContainer}>
- <Skeleton
- variant="rectangular"
- width={210}
- height={30}
- sx={{ bgcolor: "var(--light-gray)" }}
- animation={false}
- />
- // ... repeated 3 more times
+ {[...Array(4)].map((_, index) => (
+ <CommonSkeleton key={`skeleton-${index}`} />
+ ))}📝 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.
| <Skeleton | |
| variant="rectangular" | |
| width={210} | |
| height={30} | |
| sx={{ bgcolor: "var(--light-gray)" }} | |
| animation={false} | |
| /> | |
| <Skeleton | |
| variant="rectangular" | |
| width={210} | |
| height={30} | |
| sx={{ bgcolor: "var(--light-gray)" }} | |
| animation={false} | |
| /> | |
| <Skeleton | |
| variant="rectangular" | |
| width={210} | |
| height={30} | |
| sx={{ bgcolor: "var(--light-gray)" }} | |
| animation={false} | |
| /> | |
| <Skeleton | |
| variant="rectangular" | |
| width={210} | |
| height={30} | |
| sx={{ bgcolor: "var(--light-gray)" }} | |
| animation={false} | |
| /> | |
| const CommonSkeleton = () => ( | |
| <Skeleton | |
| variant="rectangular" | |
| width={210} | |
| height={30} | |
| sx={{ bgcolor: "var(--light-gray)" }} | |
| animation={false} | |
| /> | |
| ); | |
| {[...Array(4)].map((_, index) => ( | |
| <CommonSkeleton key={`skeleton-${index}`} /> | |
| ))} |
| <Skeleton | ||
| variant="rectangular" | ||
| width={210} | ||
| height={30} | ||
| sx={{ bgcolor: "var(--light-gray)" }} | ||
| animation={false} | ||
| /> | ||
| <Skeleton | ||
| variant="rectangular" | ||
| width={210} | ||
| height={30} | ||
| sx={{ bgcolor: "var(--light-gray)" }} | ||
| animation={false} | ||
| /> | ||
| <Skeleton | ||
| variant="rectangular" | ||
| width={210} | ||
| height={30} | ||
| sx={{ bgcolor: "var(--light-gray)" }} | ||
| animation={false} | ||
| /> | ||
| <Skeleton | ||
| variant="rectangular" | ||
| width={210} | ||
| height={30} | ||
| sx={{ bgcolor: "var(--light-gray)" }} | ||
| animation={false} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Yo dawg, we need to clean up this repetitive code!
These skeleton blocks are making me nervous with all this repetition. Let's make it cleaner by mapping over an array.
Here's a better way to handle it, fam:
- <Skeleton
- variant="rectangular"
- width={210}
- height={30}
- sx={{ bgcolor: "var(--light-gray)" }}
- animation={false}
- />
- // ... repeated 3 more times
+ {[...Array(4)].map((_, index) => (
+ <Skeleton
+ key={index}
+ variant="rectangular"
+ width={210}
+ height={30}
+ sx={{ bgcolor: "var(--light-gray)" }}
+ animation={false}
+ />
+ ))}📝 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.
| <Skeleton | |
| variant="rectangular" | |
| width={210} | |
| height={30} | |
| sx={{ bgcolor: "var(--light-gray)" }} | |
| animation={false} | |
| /> | |
| <Skeleton | |
| variant="rectangular" | |
| width={210} | |
| height={30} | |
| sx={{ bgcolor: "var(--light-gray)" }} | |
| animation={false} | |
| /> | |
| <Skeleton | |
| variant="rectangular" | |
| width={210} | |
| height={30} | |
| sx={{ bgcolor: "var(--light-gray)" }} | |
| animation={false} | |
| /> | |
| <Skeleton | |
| variant="rectangular" | |
| width={210} | |
| height={30} | |
| sx={{ bgcolor: "var(--light-gray)" }} | |
| animation={false} | |
| /> | |
| {[...Array(4)].map((_, index) => ( | |
| <Skeleton | |
| key={index} | |
| variant="rectangular" | |
| width={210} | |
| height={30} | |
| sx={{ bgcolor: "var(--light-gray)" }} | |
| animation={false} | |
| /> | |
| ))} |
There was a problem hiding this comment.
This can be implemented, as copy pasting Skeletons creates repetition
| <Skeleton | ||
| variant="rectangular" | ||
| width={210} | ||
| height={30} | ||
| sx={{ bgcolor: "var(--light-gray)" }} | ||
| animation={false} | ||
| /> | ||
| <Skeleton | ||
| variant="rectangular" | ||
| width={210} | ||
| height={30} | ||
| sx={{ bgcolor: "var(--light-gray)" }} | ||
| animation={false} | ||
| /> | ||
| <Skeleton | ||
| variant="rectangular" | ||
| width={210} | ||
| height={30} | ||
| sx={{ bgcolor: "var(--light-gray)" }} | ||
| animation={false} | ||
| /> | ||
| <Skeleton | ||
| variant="rectangular" | ||
| width={210} | ||
| height={30} | ||
| sx={{ bgcolor: "var(--light-gray)" }} | ||
| animation={false} | ||
| /> |
There was a problem hiding this comment.
This can be implemented, as copy pasting Skeletons creates repetition
|
@gorkem-bwl @bolatein any comments about the UI? |
Neither colors nor wording are the same. They must be same as in Figma. 
|
|
Noted! I will update right away |
Make sure you compare them side-by-side and then open your PR. |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/components/HomePageComponents/CreateActivityButton/CreateActivityButton.jsx(1 hunks)frontend/src/components/HomePageComponents/Skeletons/BannerSkeleton.jsx(1 hunks)frontend/src/components/HomePageComponents/Skeletons/CustomSkeleton.jsx(1 hunks)frontend/src/components/HomePageComponents/Skeletons/Skeleton.module.scss(1 hunks)frontend/src/scenes/dashboard/Dashboard.jsx(2 hunks)frontend/src/styles/variables.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/components/HomePageComponents/Skeletons/BannerSkeleton.jsx
- frontend/src/components/HomePageComponents/Skeletons/Skeleton.module.scss
- frontend/src/components/HomePageComponents/CreateActivityButton/CreateActivityButton.jsx
🔇 Additional comments (1)
frontend/src/scenes/dashboard/Dashboard.jsx (1)
48-49: Verify navigation path consistency
The placeholder says "Create a new helper link" but navigates to "/hint/create". Let's verify if this is intentional.
✅ Verification successful
The previous search might have failed due to quote escaping. Let's cast a wider net to find any hint-related routing or components that might help us verify the navigation consistency.
Navigation path is consistent with application routing
The navigation to "/hint/create" is correct and consistent with the application's routing structure. This path is properly defined in App.jsx and is used consistently across multiple components. The placeholder text "Create a new helper link" aligns with the hint creation functionality, as hints serve as helper content in the application.
- Route "/hint/create" is properly defined in App.jsx and maps to
- Same navigation path is used in HintDefaultPage.jsx
- Consistent with hint-related terminology used throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for route definitions and related components
rg -A 2 "'/hint/create'"
Length of output: 32
Script:
#!/bin/bash
# Let's try a broader search for hint-related routes and components
rg -i "hint.*create|create.*hint" -A 2
# Also search for route definitions
rg -i "route.*hint|hint.*route" -A 2
# Search for files with 'hint' in their name
fd -i hint
Length of output: 3292
| animation={false} | ||
| sx={{ bgcolor: "var(--gray-250)" }} |
There was a problem hiding this comment.
Hold up! Why are we disabling animations on a loading skeleton?
The animation={false} prop defeats the purpose of a loading skeleton. Loading states typically include animations to indicate activity.
- animation={false}
+ animation="wave"Also applies to: 23-24
| width={210} | ||
| height={20} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Yo! These hardcoded dimensions are making me nervous!
The fixed width (210) and height (20) values could break responsiveness on different screen sizes. Consider using relative units or props for flexibility.
- width={210}
- height={20}
+ width="100%"
+ height={height || "20px"}Committable suggestion skipped: line range outside the PR's diff.
| --gray-50: #fbfbfb; | ||
| --gray-100: #f8f8f8; | ||
| --gray-200: #C6C6C7; | ||
| --gray-250: #F3F3F3; | ||
| --blue-50: #E2EBFF; | ||
| --blue-300: #A8C1FF; |
There was a problem hiding this comment.
Mom's spaghetti moment: Dark mode support is missing!
The new color variables (--gray-* and --blue-*) don't have corresponding dark mode overrides. This could cause visibility issues in dark mode.
Add these variables to the dark mode section:
body[data-theme='dark'] {
+ --gray-50: #2c2c2c;
+ --gray-100: #1f1f1f;
+ --gray-200: #3a3a3a;
+ --gray-250: #333333;
+ --blue-50: #1a1f2e;
+ --blue-300: #2a3655;Committable suggestion skipped: line range outside the PR's diff.
| frontSkeletonProps={{ | ||
| position: "absolute", | ||
| top: 20, | ||
| left: 30, | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
These absolute positions are making my knees weak!
Using absolute positioning with hardcoded values could break layout on different screen sizes and resolutions.
Consider using CSS Grid or Flexbox for more reliable positioning:
- frontSkeletonProps={{
- position: "absolute",
- top: 20,
- left: 30,
- }}
+ className={styles.skeletonPosition}Add to your CSS module:
.skeletonPosition {
display: grid;
place-items: center;
}Also applies to: 41-45
|
I have created a new PR (#398) with the correct target branch to ensure proper integration. |
Describe your changes
Changed the button UI on homepage
Issue number
#379
Please ensure all items are checked off before requesting a review:
Before
After