452 UI improvements #453
452 UI improvements #453
Conversation
MandeepPaul
requested review from
gorkem-bwl
and removed request for
gorkem-bwl
WalkthroughThe pull request introduces modifications to several frontend components and stylesheets, focusing on enhancing component flexibility and styling. Key changes include adding a Changes
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: 4
🧹 Nitpick comments (6)
frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx (2)
41-41: Consider using clsx for className handlingFor better className management and to avoid potential undefined issues, consider using clsx or classnames library.
Example implementation:
+ import clsx from 'clsx'; // In the render - <div className={className} style={{ ...style, ...(computedFullWidth && { width: '100%' })}}> + <div className={clsx(className, 'custom-text-field')} style={{ ...style, ...(computedFullWidth && { width: '100%' })}}>🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: Expected an expression but instead found '...'.
Expected an expression here.
(parse)
Line range hint
1-134: Let's level up this component with MUI theme integration!Since this PR focuses on UI improvements, consider these architectural enhancements:
- Use MUI's styled API for consistent theming
- Document className usage in JSDoc
Example implementation:
/** * CustomTextField component * @param {Object} props * @param {string} [props.className] - Additional CSS class for custom styling * ...other props documentation */Also, consider creating a styled component:
import { styled } from '@mui/material/styles'; const StyledTextField = styled(TextField)(({ theme }) => ({ marginTop: theme.spacing(1), // ... other theme-based styles }));frontend/src/scenes/settings/ProfileTab/ProfileTab.module.css (4)
3-3: Yo dawg, that magic number's making me nervous!That 864px width is looking like a magic number, and we don't want none of that spaghetti code. Consider using a design system variable or at least documenting why this specific width was chosen.
- width: 864px; + width: var(--profile-form-width, 864px);
25-32: Caught a duplicate margin like catching mom's spaghetti!Props for using those sweet design system variables! But yo, you've got margin defined twice (lines 26 and 32). Let's clean that up.
.supportText { - margin: 0; min-width: 400px; color: var(--second-text-color); font-size: var(--font-regular); font-weight: 400; line-height: 20px; - margin-top: 6px; + margin: 6px 0 0 0; }
52-52: Let's keep these widths in sync like a rap beat!I see that 400px min-width matches the .supportText class. Let's make it a variable to keep things consistent.
+ :root { + --profile-label-width: 400px; + } .label { - min-width: 400px; + min-width: var(--profile-label-width); } .supportText { - min-width: 400px; + min-width: var(--profile-label-width); }
60-64: Clean styling like fresh Tupperware for leftover spaghetti!The textField class looks good, but there's some extra whitespace at line 64 that we can clean up.
.textField { flex-grow: 1; text-align: right; } -
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx(2 hunks)frontend/src/scenes/settings/ProfileTab/ProfileTab.jsx(3 hunks)frontend/src/scenes/settings/ProfileTab/ProfileTab.module.css(3 hunks)frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/ConfirmationPopup/ConfirmationPopup.jsx(1 hunks)frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/ConfirmationPopup/ConfirmationPopupStyles.js(1 hunks)frontend/src/templates/GuideMainPageTemplate/GuideMainPageTemplate.css(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- frontend/src/templates/GuideMainPageTemplate/GuideMainPageTemplate.css
- frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/ConfirmationPopup/ConfirmationPopupStyles.js
- frontend/src/scenes/settings/ProfileTab/ProfileTab.jsx
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx
[error] 41-41: Expected an expression but instead found '...'.
Expected an expression here.
(parse)
🔇 Additional comments (5)
frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/ConfirmationPopup/ConfirmationPopup.jsx (5)
3-3: Smooth move bringing in the custom Button, eh?Looks like you've replaced the Material-UI Button with your own from "../../../../components/Button/Button". This is in line with unifying your design system. Keep up the good work and ensure your custom Button behaves as expected in varied contexts and theming scenarios.
4-4: Nice to see the external styling approach.Pulling in
dialogStylesfrom a separate module is cleaner and helps you maintain consistent styling across components. Just make sure these styles remain flexible enough if future layout changes become necessary.
15-16: Dialling up the style with PaperProps, eh?Using
PaperPropsto add custom padding is a welcomed approach. It aligns with MUI's recommended best practices for styling and maintains a tidy component structure. No spaghetti code detected here—nice and coherent.
21-22: Solid styling with sx props.Applying
sxto bothDialogTitleandDialogContentkeeps the design consistent. It's definitely less messy than inline styling—your code is as fresh as day-old poutine!
27-39: Robust button configuration—content and variant synergy.Using your custom Button with different text,
buttonType, andvariantvalues is a slick way to maintain consistent styling and logic. “Cancel” asvariant="text"and “Confirm” asvariant="contained"is a clean approach, giving each button a distinct look while preserving design harmony. Great job trusting your custom Button to handle these props.
frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx
Outdated
Show resolved
Hide resolved
frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx
Show resolved
Hide resolved
...plates/GuideMainPageTemplate/GuideMainPageComponents/ConfirmationPopup/ConfirmationPopup.jsx
Show resolved
Hide resolved
|
Looks good @MandeepPaul - thanks. |
...s/GuideMainPageTemplate/GuideMainPageComponents/ConfirmationPopup/ConfirmationPopupStyles.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/scenes/settings/ProfileTab/ProfileTab.module.css (1)
37-37:⚠️ Potential issueStill got those fixed widths dropping like mom's spaghetti!
Previous reviewers already flagged these fixed width issues. Let's not forget their advice about using flexbox properties instead.
.photoOptions { - min-width: 350px; + flex: 1; + min-width: 0; } .label { - min-width: 400px; + flex: 0 0 auto; + margin-right: 1rem; }Also applies to: 54-54
🧹 Nitpick comments (3)
frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/ConfirmationPopup/ConfirmationPopupStyles.js (1)
1-18: Yo, the structure's clean but let's make it theme-friendly!The organization is solid, breaking down styles by component. However, we should consider using MUI's theme variables for better maintainability.
Here's how we can level up this code:
+import { useTheme } from '@mui/material/styles'; + -export const dialogStyles = { +export const useDialogStyles = () => { + const theme = useTheme(); + return { paper: { - padding: "16px", + padding: theme.spacing(2), }, title: { padding: 0, }, content: { paddingLeft: 0, }, contentText: { - fontSize: "13px", + fontSize: theme.typography.pxToRem(13), }, actions: { paddingBottom: 0, paddingRight: 0, }, + }; };frontend/src/scenes/settings/ProfileTab/ProfileTab.module.css (2)
17-17: These layout changes are straight fire, but let's make 'em responsive!The column layout and spacing look clean, but we should make it adapt to different screen sizes.
.photoAlign { display: flex; justify-content: space-between; align-items: center; + gap: 2rem; + @media (max-width: 768px) { + flex-direction: column; + align-items: flex-start; + } }Also applies to: 23-23, 25-25
62-64: Text field styling's looking fresh!The flexbox usage is on point, but let's make it more mobile-friendly.
.textField { flex-grow: 1; text-align: right; + @media (max-width: 768px) { + text-align: left; + width: 100%; + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/scenes/settings/ProfileTab/ProfileTab.jsx(3 hunks)frontend/src/scenes/settings/ProfileTab/ProfileTab.module.css(3 hunks)frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/ConfirmationPopup/ConfirmationPopup.jsx(1 hunks)frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/ConfirmationPopup/ConfirmationPopupStyles.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/scenes/settings/ProfileTab/ProfileTab.jsx
- frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/ConfirmationPopup/ConfirmationPopup.jsx
🔇 Additional comments (3)
frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/ConfirmationPopup/ConfirmationPopupStyles.js (2)
8-10: Heads up! The padding's looking kinda sus!The asymmetric padding (left: 0, others: inherited) might cause alignment issues. Consider whether this is intentional or if we should normalize the padding.
Would you like me to verify the visual impact of this asymmetric padding in the UI?
11-13: Font size check - we're on point! 🎯The 13px font size aligns with the previous discussion between @erenfn and @gorkem-bwl.
frontend/src/scenes/settings/ProfileTab/ProfileTab.module.css (1)
27-33: Clean styling with those design tokens, fam!Nice work using CSS variables for colors and font sizes - that's the way to keep it consistent!
Preview
|
Describe your changes
UI improvements
Issue number
#452
Please ensure all items are checked off before requesting a review:
MUI design popup fix
GuildeMainPageTemplate layout fix - Hint, Tour, Banner, Popup, Link
Settings page layout fix