Apply workspace page design from Figma #418
Conversation
WalkthroughThe pull request introduces several changes to the frontend of the application, primarily focusing on typography and layout enhancements. A new external stylesheet for the Pretendard font is added, and the font family settings are updated across various components. The Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
frontend/src/App.tsxOops! Something went wrong! :( ESLint: 9.12.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
frontend/src/components/layouts/WorkspaceLayout.tsx (2)
23-34: LGTM! Consider enhancing responsive paddingThe new layout structure using Stack components and maxWidth constraints is well-organized. The centered content with a maximum width of 1440px follows good desktop design practices.
Consider using theme breakpoints for responsive padding:
- <Box flexGrow={1} maxWidth="100%" px={2}> + <Box + flexGrow={1} + maxWidth="100%" + px={{ + xs: 2, + sm: 3, + md: 4 + }} + >
29-31: Document the maxWidth constantConsider adding a comment explaining why 1440px was chosen as the maximum width constraint. This helps maintain design consistency when other developers need to create similar layouts.
+ {/* Maximum content width as per Figma design specifications */} <Box mx="auto" maxWidth={1440} width="100%">frontend/index.html (2)
9-14: LGTM! Consider adding preconnect for better performance.The font stylesheet integration looks good with proper security attributes. However, to optimize performance, consider adding a preconnect hint for the CDN domain.
Add this before the font stylesheet:
+ <link rel="preconnect" href="https://cdn.jsdelivr.net" crossorigin /> <link rel="stylesheet" as="style" crossorigin href="https://cdn.jsdelivr.net/gh/orioncactus/[email protected]/dist/web/static/pretendard.min.css" />
9-14: Consider adding a font loading strategy to prevent FOUT.To prevent Flash of Unstyled Text (FOUT) and ensure a smooth font loading experience, consider implementing a font loading strategy.
Add this to manage font loading:
+ <script> + // Font loading strategy + if ("fonts" in document) { + // Optimization: Add a class when the font is loaded + document.fonts.ready.then(function () { + document.documentElement.classList.add('fonts-loaded'); + }); + } + </script> <link rel="stylesheet" as="style" crossorigin href="https://cdn.jsdelivr.net/gh/orioncactus/[email protected]/dist/web/static/pretendard.min.css" />Then in your CSS, you can handle the font loading state:
/* Add to your CSS */ body { font-family: system-ui, -apple-system, sans-serif; /* System font fallback */ } .fonts-loaded body { font-family: "Pretendard Variable", "Pretendard", system-ui, -apple-system, sans-serif; }frontend/src/components/cards/DocumentCard.tsx (2)
1-5: Consider organizing imports for future implementationWhile the current imports are clean, there are several commented-out components (IconButton, MoreVertIcon, Chip, AvatarGroup, Avatar) that will need to be imported when the TODOs are implemented. Consider adding these imports but commenting them out to make future implementation easier.
import { Paper, Stack, Typography } from "@mui/material"; import { Link, useParams } from "react-router-dom"; import { Document } from "../../hooks/api/types/document.d"; import moment from "moment"; import AccessTimeIcon from "@mui/icons-material/AccessTime"; +// TODO: Uncomment when implementing related features +// import { IconButton, Chip, Avatar, AvatarGroup } from "@mui/material"; +// import MoreVertIcon from "@mui/icons-material/MoreVert";
34-37: Enhance TODO comments with issue referencesThe TODO comments are clear but would benefit from being linked to specific issues for better tracking. Consider creating issues for:
- Document deletion feature
- Tag system implementation
- Yorkie presence integration
-{/* TODO(devleejb): When the document deletion is implemented, uncomment the following code */} +{/* TODO(devleejb): When the document deletion is implemented (issue #XXX), uncomment the following code */}Also applies to: 64-84
frontend/src/App.tsx (1)
66-66: Consider adding font loading status check.To prevent layout shifts and ensure optimal user experience, consider adding a font loading check using the FontFace API.
Here's a suggested implementation to add in your component:
useEffect(() => { document.fonts.ready.then(() => { // Font is loaded and ready // You can set a state here to trigger a re-render or update loading status }).catch((error) => { console.error('Font loading failed:', error); // Handle the error appropriately }); }, []);frontend/src/pages/workspace/Index.tsx (4)
74-76: Consider making the sticky positioning more maintainableThe hardcoded
top: 64value assumes a fixed header height. Consider using a CSS variable or theme value for better maintainability.- top: 64, + top: 'var(--header-height)',
89-96: Add accessibility attributes to the New Note buttonConsider adding an aria-label to improve accessibility for screen readers.
<Button variant="contained" startIcon={<AddIcon />} onClick={handleCreateDocumentModalOpen} + aria-label="Create new note" > New Note </Button>
110-112: Consider improving loader visibilityThe current loader positioning might not be obvious to users. Consider adding margin and a loading message for better user experience.
- <Stack className="loader" key={0} alignItems="center"> + <Stack className="loader" key={0} alignItems="center" my={4}> <CircularProgress size={20} /> + <Typography variant="body2" color="text.secondary" mt={1}> + Loading more documents... + </Typography> </Stack>
70-128: Consider implementing an error boundaryWhile the component handles loading states, it might benefit from proper error handling using React Error Boundaries to gracefully handle runtime errors.
Would you like me to provide an example implementation of an Error Boundary component?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
frontend/index.html(1 hunks)frontend/src/App.tsx(1 hunks)frontend/src/components/cards/DocumentCard.tsx(2 hunks)frontend/src/components/layouts/WorkspaceLayout.tsx(1 hunks)frontend/src/index.css(1 hunks)frontend/src/pages/workspace/Index.tsx(2 hunks)frontend/src/pages/workspace/member/Index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/src/pages/workspace/member/Index.tsx
- frontend/src/index.css
🔇 Additional comments (6)
frontend/src/components/cards/DocumentCard.tsx (4)
16-21: LGTM! Clean navigation implementation
The switch to using Link component instead of click handlers is a good improvement for accessibility and semantic HTML.
22-28: Verify padding values against Figma design
The Paper component styling looks good, but let's ensure the padding values (px: 3, py: 2) match exactly with the Figma specifications from issue #375.
42-51: Review fixed height constraint on tags container
The Stack component has a fixed height of 56px which might cause issues if there are multiple rows of tags. Consider using min-height or max-height instead to handle varying content better.
-<Stack direction="row" mt={1} gap={1} flexWrap="wrap" height={56}>
+<Stack direction="row" mt={1} gap={1} flexWrap="wrap" minHeight={56}>29-33: LGTM! Consistent date formatting
The switch to "D MMM YYYY" format for both creation and update times improves consistency across the UI.
Also applies to: 52-63
frontend/src/App.tsx (1)
66-66: LGTM! Verify font loading configuration.
The font family configuration is well-structured with appropriate fallbacks for different operating systems and proper internationalization support.
Let's verify the font configuration:
✅ Verification successful
Font configuration is properly set up
The Pretendard font is correctly configured:
- CDN import via
pretendard.min.cssinindex.html - Font family fallback chain properly defined in both
index.cssand theme configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check font loading configuration
# Expected: Find font import in index.html or CSS files
# Search for Pretendard font configuration
echo "Checking for font configuration..."
rg -l "Pretendard" --type html --type css
# Search for @font-face or import statements
echo "Checking for font loading methods..."
rg "@(font-face|import.*Pretendard)" --type html --type css
Length of output: 288
Script:
#!/bin/bash
# Let's check the content of index.css to see how Pretendard is configured
echo "Content of frontend/src/index.css:"
cat frontend/src/index.css
# Also search for any CDN or font imports in HTML files
echo -e "\nChecking for CDN imports in HTML files:"
rg "link.*rel=.*stylesheet.*Pretendard|script.*src.*Pretendard" --type html
Length of output: 757
Script:
#!/bin/bash
# Let's check the HTML file for any font preloading or stylesheet imports
echo "Checking index.html content:"
cat frontend/public/index.html || cat frontend/index.html
# Also check for any font imports in other CSS files
echo -e "\nChecking for font imports in all CSS files:"
fd -e css -x grep -l "Pretendard"
Length of output: 2171
frontend/src/pages/workspace/Index.tsx (1)
26-29: Consider removing or implementing the commented "TABLE" tab
The commented "TABLE" tab suggests an incomplete feature. Either implement the table view functionality or remove the commented code to maintain clean production code.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #375
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Chores