#434: Popup Dialog's component #451
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Sequence DiagramsequenceDiagram
Parent Component->>PopUpMessages: Render with props
PopUpMessages->>PopUpMessages: Process props
PopUpMessages-->>Parent Component: Display customized message
The sequence diagram illustrates how the new Possibly related PRs
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: 0
🧹 Nitpick comments (6)
frontend/src/components/PopUpMessages/PopUpMessages.jsx (3)
36-38: Adaptive width
Limiting width per type is a neat idea. For better responsiveness on smaller screens, you could use media queries.
40-42: Conditional margins
This approach works but might cause layout shifts. Keep an eye on consistency across various screen sizes.
44-65: Neat structure and accessible design
The UI is tidy. Consider addingaria-labelor other accessibility attributes to ensure everyone can enjoy your popup.frontend/src/components/PopUpMessages/PopUpMessages.module.scss (3)
32-44: .textContainer
Clear structure. Thetext-stylemixin is neat. Make sure these headings scale gracefully.
48-53: .buttons for better UX
A row layout with spacing is fab. Could consider a wrap for small screens.
82-87: .closeBtn accessibility
This button stands out for quick dismissals. Consider adding anaria-labelfor better screen reader support.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/PopUpMessages/PopUpMessages.jsx(1 hunks)frontend/src/components/PopUpMessages/PopUpMessages.module.scss(1 hunks)
🔇 Additional comments (10)
frontend/src/components/PopUpMessages/PopUpMessages.jsx (4)
1-4: Imports look fresh and clean
No issues spotted here. Keep on rockin’ these dependencies.
6-13: Default props provide clarity
These defaults simplify usage and make the component more robust. Nicely done.
14-35: Handling unknown message types
Returning null when the type is unexpected might surprise users. Consider a fallback icon or an explanatory message.
67-67: Export is smooth
Exporting at the bottom is a common practice that helps with readability. Stay the course!
frontend/src/components/PopUpMessages/PopUpMessages.module.scss (6)
1-1: Global styles import
Using SCSS modules plus a global import is a sweet combo. Good job bridging these styles.
3-17: .container positioning
Absolute placement works, but test on various viewports to ensure it doesn’t clip or stack awkwardly.
19-28: .iconAndText arrangement
The horizontal flex layout looks jammin’. Makes sense for icons and text side by side.
55-63: Shared button styles
Merging .acceptButton and .dismiss rules is practical. Reduces duplication and keeps it all tidy.
65-69: .acceptButton styles
The added background and padding create strong emphasis for the acceptance path. Works fine.
71-79: .dismiss hover
The hover effect adds subtle interactivity. Nicely done.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/components/PopUpMessages/PopUpMessages.jsx (1)
48-63: Ensure event listener behaviour is robust.While the solution works well for dismissing the popup outside its boundaries, kindly watch out for potential performance concerns when adding global event listeners if this pattern is repeated throughout the application. Otherwise, your code remains as steady as a well-cooked pot of mom’s spaghetti.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/PopUpMessages/PopUpMessages.jsx(1 hunks)frontend/src/components/PopUpMessages/usePopUp.js(1 hunks)
🔇 Additional comments (2)
frontend/src/components/PopUpMessages/usePopUp.js (1)
1-14: Solid introduction of a custom hook.This code is clean and simple, so there’s no need for sweaty palms or trembling knees here—this hook does exactly what’s advertised without extra overhead. Nicely done.
frontend/src/components/PopUpMessages/PopUpMessages.jsx (1)
65-65: Conditional rendering is on point.Conditionally returning
nullwhenisOpenisfalseis a great approach to avoid rendering overhead—definitely no sign of shaky execution here.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/src/components/PopUpMessages/PopUpMessages.jsx (2)
17-38: This switch statement's heavier than a plate of spaghetti! Let's make it lighter! 🍝Consider refactoring the icon logic using an object lookup and moving those hardcoded colors to a constants file.
+const POPUP_TYPES = { + INFO: 1, + ERROR: 2, + SUCCESS: 3, +}; + +const ICON_COLORS = { + ERROR: "#D92D20", + SUCCESS: "#079455", +}; + +const ICONS_MAP = { + [POPUP_TYPES.INFO]: (props) => <InfoOutlinedIcon {...props} />, + [POPUP_TYPES.ERROR]: (props) => ( + <ErrorOutlineOutlinedIcon {...props} style={{ color: ICON_COLORS.ERROR }} /> + ), + [POPUP_TYPES.SUCCESS]: (props) => ( + <ErrorOutlineOutlinedIcon {...props} style={{ color: ICON_COLORS.SUCCESS }} /> + ), +}; + const getIcon = () => { - switch (type) { - case 1: - return <InfoOutlinedIcon className={styles.icon} />; - // ... rest of switch - } + const IconComponent = ICONS_MAP[type]; + return IconComponent ? <IconComponent className={styles.icon} /> : null; };
39-45: These styles are scattered like dropped spaghetti! Let's clean it up! 🧹Move these inline styles to your SCSS module and use CSS custom properties for the magic numbers.
// In PopUpMessages.module.scss +:root { + --popup-max-width-default: 30rem; + --popup-max-width-compact: 20rem; + --message-margin-default: 0.5rem 0; +} +.container { + &[data-type="4"], + &[data-type="5"] { + max-width: var(--popup-max-width-compact); + } + &:not([data-type="4"]):not([data-type="5"]) { + max-width: var(--popup-max-width-default); + } +} +.message { + margin: var(--message-margin-default); + &[data-type="5"] { + margin: 0; + } +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/PopUpMessages/PopUpMessages.jsx(1 hunks)
🔇 Additional comments (2)
frontend/src/components/PopUpMessages/PopUpMessages.jsx (2)
83-89: The close button's accessibility is already cooking! Bon appétit! 👨🍳Nice job adding the aria-label to the close button! That's some good accessibility pasta right there!
75-78:⚠️ Potential issueThese buttons are like meatballs without sauce - they need some handlers! 🍝
The left and right buttons are missing onClick handlers. Also, consider adding aria-labels for better accessibility.
<div className={styles.buttons}> <button className={styles.dismiss} + onClick={onLeftButtonClick} + aria-label={leftBtnText} > {leftBtnText} </button> <button className={styles.acceptButton} + onClick={onRightButtonClick} + aria-label={rightBtnText} > {rightBtnText} </button> </div>Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/components/PopUpMessages/PopUpMessages.jsx (3)
18-39: Let's clean up this icon logic faster than mom's spaghetti sauce! 🍝The switch statement could use some cleanup to avoid repeating ourselves. Also, those magic numbers are making me nervous like knees weak, arms heavy!
Here's a cleaner approach:
+const ICON_CONFIGS = { + 1: { icon: InfoOutlinedIcon, color: undefined }, + 2: { icon: ErrorOutlineOutlinedIcon, color: "#D92D20" }, + 3: { icon: ErrorOutlineOutlinedIcon, color: "#079455" } +}; const getIcon = () => { - switch (type) { - case 1: - return <InfoOutlinedIcon className={styles.icon} />; - case 2: - return ( - <ErrorOutlineOutlinedIcon - className={styles.icon} - style={{ color: "#D92D20" }} - /> - ); - case 3: - return ( - <ErrorOutlineOutlinedIcon - className={styles.icon} - style={{ color: "#079455" }} - /> - ); - default: - return null; - } + const config = ICON_CONFIGS[type]; + if (!config) return null; + + const IconComponent = config.icon; + return ( + <IconComponent + className={styles.icon} + style={config.color ? { color: config.color } : undefined} + /> + ); };
40-46: These styles are spread like sauce on spaghetti! 🍝Let's move these inline styles to the SCSS module and use constants for better maintainability.
Consider moving these styles to your SCSS module:
// In PopUpMessages.module.scss .container { &--compact { max-width: 20rem; } &--default { max-width: 30rem; } } .message { margin: 0.5rem 0; &--compact { margin: 0; } }Then update your JSX:
-const containerStyle = { - maxWidth: type === 4 || type === 5 ? "20rem" : "30rem", -}; - -const messageStyle = { - margin: type === 5 ? "0" : "0.5rem 0px", -}; +const containerClass = `${styles.container} ${ + (type === 4 || type === 5) ? styles['container--compact'] : styles['container--default'] +}`; + +const messageClass = `${styles.message} ${ + type === 5 ? styles['message--compact'] : '' +}`;
48-64: These comments are as casual as eating spaghetti with your hands! 🍝While the code is solid, let's make those comments more professional than a five-star Italian restaurant.
-//Close PopUp when we click outside of the component +// Handle click events outside the popup component to trigger closure -//Add callback separately so we can cleanup later +// Define callback separately for proper cleanup in useEffect -//CLEAN UP to prevent memory leaks +// Cleanup event listener to prevent memory leaks
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/PopUpMessages/PopUpMessages.jsx(1 hunks)
🔇 Additional comments (2)
frontend/src/components/PopUpMessages/PopUpMessages.jsx (2)
1-17: Yo, these imports and props are cleaner than mom's kitchen! 🍝The component setup is looking fresh with proper imports and clean default values. Props declaration is neat and tidy - no spaghetti code here!
95-103: These PropTypes are as well-organized as mom's recipe book! 🍝The prop validation is looking clean and proper. Nice job making the essential props required!
|
Hey, I realized this was taking some time. We have a release in one week, so we are trying to finish all the issues before then. Mandeep from our internal team solved the issue in #464. But thank you so much for your time |
#434
Created component for pop up dialogs.
Added some styling and also header, content, leftButtonText, and rightButtonText as props.
I couldn't include the component inside the app as it was hard for me to see realtime changes when a file was modified
but I was able to test it in some separate environment.
Any suggestion for improvement is welcomed.