Add Colored Labels for HTTP Status Codes #972
Conversation
WalkthroughThe changes introduce a new Changes
Assessment against linked issues
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: 3
🧹 Outside diff range and nitpick comments (2)
Client/src/Components/HttpStatusLabel/index.jsx (1)
1-13: Yo, dawg! Let's beef up that JSDoc, it's lookin' a bit slim!The imports are on point, but the JSDoc could use some extra sauce. How about we add more deets to make it pop?
Here's a suggestion to make your JSDoc as tasty as mom's spaghetti:
/** * @component HttpStatusLabel * @description Renders a colored label for HTTP status codes. * @param {Object} props * @param {number|string} props.status - The HTTP status code or "unresolved" * @returns {JSX.Element} A ColoredLabel component with appropriate styling * @example * // Render a 400 HTTP status code label * <HttpStatusLabel status={400} /> * * // Render an unresolved status label * <HttpStatusLabel status="unresolved" /> */This'll make your code documentation as fire as your rhymes, homie!
Client/src/Pages/Incidents/IncidentTable/index.jsx (1)
187-191: Aight, this status code rendering is fire!Yo, this conditional rendering is smoother than mom's spaghetti! It's handling those status codes like a pro, using that new
HttpStatusLabelcomponent we just imported. That's gonna make the UI pop like it's on stage at the rap battle!Just one tiny thing to make it even more lit:
Consider wrapping that "N/A" in a Typography component for consistency, like this:
{check.statusCode ? ( <HttpStatusLabel status={check.statusCode} /> ) : ( - "N/A" + <Typography variant="body2">N/A</Typography> )}This'll make sure the "N/A" text style matches the rest of your UI. It's all about that consistent flow, you feel me?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- Client/src/Components/HttpStatusLabel/index.jsx (1 hunks)
- Client/src/Pages/Incidents/IncidentTable/index.jsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
Client/src/Components/HttpStatusLabel/index.jsx (1)
35-40: Yo, this return statement's as smooth as mom's spaghetti!Your component's serving up that
ColoredLabellike it's a hot plate, and I'm diggin' it!That
String(status)conversion? That's some smart thinking right there, making sure we don't get caught with our pants down if someone throws a number at us. Keep spittin' that fire code, homie!Client/src/Pages/Incidents/IncidentTable/index.jsx (1)
22-22: Yo, this import's lookin' fresh!The import for
HttpStatusLabelis on point, dawg. It's following the project's structure like it's following the beat. Keep it up!
|
Just for my info - is there a reason the stroke color of the labels is different from what I see under the status column, e.g those with pink background? Also is there a better font color for the labels in yellow bg, as they are barely readable? |
|
Hi @gorkem-bwl
I'm reusing the
Thank you for the feedback. I made some color changes. I hope it meets your approval!
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- Client/src/Components/HttpStatusLabel/index.jsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
Client/src/Components/HttpStatusLabel/index.jsx (4)
1-14: Yo, these imports and docs are tight as mom's spaghetti!The imports are on point, and that JSDoc's got more flow than Eminem. We're off to a solid start, homie!
15-25: Yo, these colors are poppin' like a fresh beat!Mad props for movin' that
getColorsfunction outside the component, dawg. You're keepin' it fresh and efficient!Just one thing's makin' my palms sweaty:
textColor: theme.palette.warning.text, },You sure you meant to use
warning.textinstead ofwarning.main? It might be intentional, but let's make sure we ain't trippin'.
27-32: Yo, this function's handling status codes like a boss!This
getGenericStatusfunction's got more swagger than a rap battle! It's catchin' all them status codes and handlin' 'em like a pro. You've taken that past feedback and turned it into pure fire!
34-49: Damn, this component's fresher than new kicks!Your
HttpStatusLabelcomponent's got more style than a platinum record! It's usin' them helper functions like a pro and servin' up them colored labels hotter than mom's spaghetti. You've nailed them PR objectives like they were the last verse in 8 Mile!
Currently the labels look different from what the status labels (the background is ok, stroke is not). Could you let me know why that happens? |
I’ll try to explain: Basically, we have 2 components in the app: BaseLabel and ColoredLabel. The BaseLabel allows you to pass the style and specify the color for each piece of information: background, stroke, and text. (This is used in the Status Label). This way, you need to provide all 3 colors to the BaseLabel. On the other hand, the ColoredLabel only allows you to pass a single color, and it calculates the background and stroke colors. (This is the one I chose to use). Why did I opt for the ColoredLabel? It was already being exported, and I wanted to reuse it with minimal changes. I can use the BaseLabel and make it identical to the Status Label if you prefer. Would you like that change? |
|
Understood. Thanks for the detailed response.
Yes, please. That way it would look more consistent. |
|
All done! |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- Client/src/Components/HttpStatusLabel/index.jsx (1 hunks)
- Client/src/Components/Label/index.jsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
Client/src/Components/HttpStatusLabel/index.jsx (3)
1-13: Yo, these imports and docs are tight, homie!The imports are on point, and that JSDoc's got more flow than Eminem in 8 Mile. We're startin' strong, no weak knees here!
33-38: Yo, thisgetGenericStatusfunction is droppin' bars!You've handled them edge cases like they're hot mics, dawg. This function's got more flow than a freestyle battle. Keep it up!
40-59: This component's got more style than Slim Shady!Your
HttpStatusLabelis layin' down beats smoother than a DJ. It's usin' them hooks and helpers like a pro, and thatBaseLabelis dressed to impress. You're killin' it, homie!Client/src/Components/Label/index.jsx (1)
194-194: Yo, this export's got me nervous, but it's straight fire! 🔥Aight, so we're exportin' BaseLabel now. That's cool, but my palms are sweaty thinkin' about how this might shake up the whole codebase, you feel me?
Let's make sure we ain't messin' up nobody's code. Run this script to check if BaseLabel's bein' used right:
If this script shows any weird stuff, we might need to clean up our act in other parts of the code, you know what I'm sayin'?
|
Looks great! Thank you. |
|
@rog404 You should be able to just add an additional prop to enable or disable the dot and reuse the same component. |
|
You could be right. My only concern is not to make this component too large since I will have to include the color codes for HTTP status within the Label. In this case, I would need to understand the best way to know when the status will be taken into consideration. I want to understand, actually, if it makes sense to take this complexity into the component and make it less generic for other cases. Another alternative would be to remove the 'colors from the component' and let the person who wants the labels know how to work with them. What do you think? |
I think for now we can just use the It may make sense to refactor this afterwards to allow the user to select what colors they want and pass that in to the component via props. We can explore that after in a new issue. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
Client/src/Components/Label/index.jsx (1)
167-167: Default color game strong, but let's make it bulletproof!Yo, that default color handling is smoother than a fresh jar of Skippy. But what if we accidentally pass in a whack status? We gotta make sure our code doesn't choke like MC at their first rap battle.
Let's add a console warning to keep our devs in check:
-const { borderColor, bgColor, dotColor } = colors[status] || colors["cannot resolve"]; +const statusColors = colors[status] || colors["cannot resolve"]; +if (!colors[status]) { + console.warn(`Unknown status: ${status}. Falling back to "cannot resolve" colors.`); +} +const { borderColor, bgColor, dotColor } = statusColors;This way, we're catching those sneaky bugs faster than Eminem catches a beat. You dig?
Client/src/Pages/Incidents/IncidentTable/index.jsx (1)
191-197: Yo, this status code glow-up is fire, but we can make it even hotter!Aight, check it. You've taken those boring ol' status codes and turned them into some fresh labels. That's dope! But peep this:
- You're using
getGenericStatusfor thestatusprop, which is tight for the color.- You're showing the original
check.statusCodeas text, keeping all the deets. Smart move!But here's where we can level up:
<StatusLabel status={getGenericStatus(check.statusCode)} text={check.statusCode || "Unknown"} customStyles={{ textTransform: "capitalize" }} />See that? Instead of "N/A", we're using "Unknown". It's like swappin' out mom's spaghetti for some gourmet pasta, you feel me? It's clearer and looks more pro.
Also, don't forget to update that
getGenericStatusfunction like we talked about earlier. If we make those changes, this code's gonna be smoother than a fresh beat!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- Client/src/Components/Label/index.jsx (1 hunks)
- Client/src/Pages/Incidents/IncidentTable/index.jsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
Client/src/Components/Label/index.jsx (1)
Line range hint
1-205: Yo, this code is fire, but let's make it a platinum record!Overall, this update is tighter than Eminem's rhymes in "Rap God". We've got new status codes, a dope 'hasDot' prop, and some slick color handling. But like any good remix, we can still drop a few more beats to make it perfect.
Let's recap the main points:
- The 'hasDot' prop is a great addition, but we need to ensure it's used consistently across the project.
- We should add colors for more 4xx status codes to cover all our bases.
- The default color handling could use a little warning system to catch any stray statuses.
- The PropTypes need a bit of tweaking to match real HTTP status codes.
If we make these changes, this code will be dropping beats harder than Dr. Dre in his prime. You feel me?
rog404
force-pushed
the
rog404/http-status-label
branch
from
83e6b26 to
df83f44
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Client/src/Components/Label/index.jsx (2)
152-161: These new status codes are straight fire, but we're missing a beat!Yo, adding colors for 400 and 500 status codes is tight, but we're forgetting about their homies. What about 401, 403, and 404? They're feeling left out like backup dancers at a Beyoncé concert.
Let's add some love for the other 4xx codes:
dotColor: theme.palette.warning.main, bgColor: theme.palette.warning.bg, borderColor: theme.palette.warning.light, }, + 401: { + dotColor: theme.palette.warning.main, + bgColor: theme.palette.warning.bg, + borderColor: theme.palette.warning.light, + }, + 403: { + dotColor: theme.palette.warning.main, + bgColor: theme.palette.warning.bg, + borderColor: theme.palette.warning.light, + }, + 404: { + dotColor: theme.palette.warning.main, + bgColor: theme.palette.warning.bg, + borderColor: theme.palette.warning.light, + }, dotColor: theme.palette.error.main, bgColor: theme.palette.error.bg, borderColor: theme.palette.error.light, },This way, we're covering all the bases like Eminem covers syllables in a verse. You feel me?
182-190: This 'hasDot' implementation is droppin' beats like it's hot!Yo, this conditional rendering of the dot is smoother than Eminem's flow in "Rap God". It's gonna make those labels pop like the bass in a Dr. Dre track!
But hold up, we gotta think about our visually impaired homies too. Let's add some aria attributes to make it accessible:
{hasDot && ( <Box width={7} height={7} bgcolor={dotColor} borderRadius="50%" marginRight="5px" + role="presentation" + aria-hidden="true" /> )}Now we're spittin' fire and being inclusive at the same time. That's how we roll in the code game, ya dig?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- Client/src/Components/Label/index.jsx (5 hunks)
- Client/src/Pages/Incidents/IncidentTable/index.jsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Client/src/Pages/Incidents/IncidentTable/index.jsx
🧰 Additional context used
🔇 Additional comments (2)
Client/src/Components/Label/index.jsx (2)
122-122: Yo, this 'hasDot' prop is straight fire!Adding the 'hasDot' prop to the JSDoc is like droppin' a sick beat in your track. It's gonna make developers' lives easier than Eminem's rhyme flow. Keep that documentation fresh, homie!
129-129: This signature update is smoother than Slim Shady's flow!Adding that 'hasDot' prop with a default value of false? That's like giving your component a safety net, just in case it falls on its face. It's gonna keep your code running smoother than Eminem's rhymes in "Lose Yourself". Nice work, homie!
rog404
force-pushed
the
rog404/http-status-label
branch
from
9953074 to
15c4d2b
Compare
|
@ajhollid Can you help me with this conflict? Every time I resolve this conflict, the code seems to lose its proper indentation. When I use git diff before committing, the code looks correct. This is making it difficult to keep the PR clear for reviewing the changed files. |
We use prettier to format, you should be able to just call What IDE are you using? Maybe you've got something overriding our formatting? |
|
I've merged develop in for you, I think that's what you want. You can pull that and check. @marcelluscaio has some ideas he'd like to share with you about this PR however, so please touch base with him and we'll go from there 👍 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
Client/src/Pages/Incidents/IncidentTable/index.jsx (1)
22-53: Yo, dawg! Let's beef up them prop types, you feel me?Listen up, homie. That BaseLabel component? It's tight, but we can make it tighter. Check it:
- That
stylesprop? It's like mom's spaghetti - kinda vague. Let's break it down and make it specific, ya dig?- We're missin' some default props. It's like forgetting the sauce on your spaghetti, know what I'm sayin'?
Here's how we can clean it up:
BaseLabel.propTypes = { label: PropTypes.string.isRequired, styles: PropTypes.shape({ color: PropTypes.string, backgroundColor: PropTypes.string, borderColor: PropTypes.string, // Add any other style props you're using }), children: PropTypes.node, }; BaseLabel.defaultProps = { styles: {}, children: null, };This way, we're keepin' it real with what styles we expect, and we're not gonna lose our cool if someone forgets to pass some props. It's all about that consistency, you feel me?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- Client/src/Components/Label/index.jsx (3 hunks)
- Client/src/Pages/Incidents/IncidentTable/index.jsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Client/src/Components/Label/index.jsx
🧰 Additional context used
|
I'll wait for @marcelluscaio ideas then. He can contact me if he needs anything!" If you need help with anything else, just let me know! |
Great, appreciate your patience, the team should get back to you after the weekend. |
|
Hi, @rog404 ! Sorry it took me a while to get back. I really liked your approach in the lighten function! |
rog404
force-pushed
the
rog404/http-status-label
branch
from
6151ce2 to
7d8c878
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
Client/src/Pages/Incidents/IncidentTable/index.jsx (1)
192-197: Yo, this StatusLabel's got some fresh sauce, but we can make it even tastier!Aight, check it out. You've spiced up that StatusLabel like it's a gourmet plate of spaghetti. I'm diggin' the new flavor, but let's add a pinch more seasoning:
- That
getGenericStatuscall? It's solid, like al dente pasta.- Setting
hasDotto false? Smart move, keeping it clean like a fresh tablecloth.- Converting the status code to a string? That's some chef's kiss stuff right there.
But yo, we can make this dish even more delicious. Instead of
String(check.statusCode || "N/A"), how about we use a template literal? It's like using a fancy pasta cutter:text={`${check.statusCode ?? "N/A"}`}This way, we're using modern JavaScript to handle that potential
undefinedcase. It's like serving up mom's spaghetti with a gourmet twist!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- Client/src/Components/Label/index.jsx (5 hunks)
- Client/src/Pages/Incidents/IncidentTable/index.jsx (2 hunks)
🧰 Additional context used
|
@ajhollid But I believe is when you add a wrong url.
|
OK ill do some testing as soon as I can. In the meantime you could just display a default color for incidents where a code doesn't exist. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
Client/src/Components/HttpStatusLabel/index.jsx (3)
16-21: Yo, let's give this function a name that spits straight fire!The function's doing the Lord's work, but its name could be more descriptive about what it's really doing - getting that base HTTP status category.
-const getRoundedStatusCode = (status) => { +const getHttpStatusCategory = (status) => {
59-63: Yo, let's not let this TODO slip through the cracks like Mom's spaghetti!That TODO about restricting status types needs some tracking love. We should create an issue to handle this after the IncidentTable situation gets sorted.
Want me to create a GitHub issue to track this TODO? I can link it to the IncidentTable work too!
23-57: This component's flow is smoother than a fresh jar of Skippy!Your implementation is clean and focused, using BaseLabel for consistency. The color mapping strategy makes it easy to extend for other status codes too.
Consider adding this component to your design system documentation so other devs know it's the go-to solution for HTTP status displays.
Client/src/Components/Label/index.jsx (1)
195-195: Yo, this export is just the first verse of our refactoring rap! 🎤Based on the PR discussion, we're planning to split this into separate
StatusLabelandHttpStatusLabelcomponents that both extendBaseLabel. This export change is setting us up for that next track.Consider these next steps:
- Create a new
HttpStatusLabelcomponent extendingBaseLabel- Move HTTP status-specific logic to the new component
- Keep
StatusLabelfocused on up/down/paused statesWould you like me to help draft the new component structure?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
- Client/src/Components/HttpStatusLabel/index.jsx (1 hunks)
- Client/src/Components/Label/index.jsx (3 hunks)
- Client/src/Pages/Incidents/IncidentTable/index.jsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Client/src/Pages/Incidents/IncidentTable/index.jsx
🔇 Additional comments (2)
Client/src/Components/HttpStatusLabel/index.jsx (1)
25-41: Let's make sure these theme colors are ready to drop the beat!Your color mapping is tight, but we gotta verify these theme palette values exist, especially them
.bgand.lightvariants.✅ Verification successful
The previous regex had issues, so I've adjusted it to properly search for theme palette definitions. I'm also searching for theme-related files to ensure we catch all possible theme configurations. This will help us verify if the required color variants (main, bg, light) exist for warning, error, and unresolved palettes.
Yo, we need to check if these theme files got the right color variants we're looking for - the main, bg, and light variants for warning, error, and unresolved. Let's drop this beat and see what we find in those theme files!
Yo, your theme palette is straight fire! 🔥
The theme's got all the right ingredients - every color variant you're using (main, bg, light) is properly defined in both light and dark themes for warning, error, and unresolved states. Your palette's cooked to perfection, just like mom's spaghetti!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for theme palette definitions to ensure all required colors exist rg -A 10 "palette.*=.*{" --glob "*.{js,jsx,ts,tsx}"Length of output: 143
Script:
#!/bin/bash # Let's fix that regex and search for theme palette definitions rg -A 10 "palette\s*=\s*[{]|createTheme.*[{]" --glob "*.{js,jsx,ts,tsx}" # Also search for any theme-related files fd -e js -e jsx -e ts -e tsx "theme"Length of output: 1783
Script:
#!/bin/bash # Let's dive deeper into both theme files to check for our specific color variants cat Client/src/Utils/Theme/lightTheme.js Client/src/Utils/Theme/darkTheme.js | rg -A 5 "(warning|error|unresolved).*:\s*[{]" # Also check if these colors are defined elsewhere in the theme files cat Client/src/Utils/Theme/lightTheme.js Client/src/Utils/Theme/darkTheme.js | rg "bg:|light:"Length of output: 1596
Client/src/Components/Label/index.jsx (1)
166-166: This fallback logic is straight fire! 🔥Clean implementation using object destructuring with a solid fallback to "cannot resolve" colors.
rog404
force-pushed
the
rog404/http-status-label
branch
from
c992379 to
95a0ad0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
Client/src/Components/HttpStatusLabel/index.jsx (1)
59-63: Let's not let this TODO slip through the cracks like loose spaghetti!That TODO about restricting status types needs to be tracked properly, fam. We should create an issue to handle this technical debt.
Want me to create a GitHub issue to track this TODO? I can help set up the task to restrict the status prop to only accept numeric values once the "N/A" status situation in IncidentTable is resolved.
Client/src/Components/Label/index.jsx (1)
148-157: Time to split this track into separate beats! 🎵Yo, mixing HTTP status codes into StatusLabel is like trying to fit a rap verse in a country song - it works, but it ain't right! Let's restructure this joint:
- Create a separate
HttpStatusLabelcomponent that extendsBaseLabel- Move HTTP status code handling to
HttpStatusLabel- Keep
StatusLabelfocused on up/down/paused statesHere's the blueprint for this remix:
// HttpStatusLabel.jsx const HttpStatusLabel = ({ status, text, customStyles }) => { const theme = useTheme(); const colors = { dotColor: theme.palette.warning.main, bgColor: theme.palette.warning.bg, borderColor: theme.palette.warning.light, }, dotColor: theme.palette.error.text, bgColor: theme.palette.error.bg, borderColor: theme.palette.error.light, } }; // ... rest of the implementation };This way, each component drops its own beat without stepping on others' toes! 🎤
Also applies to: 195-195
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- Client/src/Components/HttpStatusLabel/index.jsx (1 hunks)
- Client/src/Components/Label/index.jsx (3 hunks)
- Client/src/Pages/Incidents/IncidentTable/index.jsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Client/src/Pages/Incidents/IncidentTable/index.jsx
🔇 Additional comments (3)
Client/src/Components/HttpStatusLabel/index.jsx (2)
5-14: Yo, that documentation example's still not keeping it real!That JSDoc example's still showing the wrong type, fam. We already dropped this beat before, but it's still showing
status="404"when it should bestatus={404}.
23-57: Yo, this implementation's actually fresher than we thought!I see that previous review suggested moving the colors object outside, but keeping it inside is actually the right move here since it needs that sweet theme access. The implementation's clean and the color mapping's on point!
Client/src/Components/Label/index.jsx (1)
166-166: Fallback game strong! 💪That fallback to "cannot resolve" colors is clean like a fresh beat - exactly what we need when we hit an unknown status!
|
What I did here was create only the new I aimed to keep modifications in unrelated files to a minimum, making the PR concise for faster review. Do you think prioritizing these refactors is necessary right now, or would it be more beneficial to address other issues that have a direct impact on the app? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
Client/src/Components/Label/index.jsx (4)
Line range hint
22-59: Yo dawg, let's make these PropTypes as tight as Eminem's rhymes! 🎤The
BaseLabelcomponent is lookin' fresh, but we can make it even fresher by beefin' up those PropTypes! Here's how we drop this beat:BaseLabel.propTypes = { label: PropTypes.string.isRequired, styles: PropTypes.shape({ color: PropTypes.string, backgroundColor: PropTypes.string, + borderColor: PropTypes.string, + borderRadius: PropTypes.oneOfType([ + PropTypes.number, + PropTypes.string + ]), + padding: PropTypes.oneOfType([ + PropTypes.number, + PropTypes.string + ]), }), children: PropTypes.node, };Also, let's update that JSDoc to include all the dope style options we're supporting:
/** * @typedef {Object} Styles * @param {string} [color] - The text color * @param {string} [backgroundColor] - The background color * @param {string} [borderColor] - The border color * @param {(string|number)} [borderRadius] - The border radius * @param {(string|number)} [padding] - The padding */
Line range hint
61-106: Let's make these color calculations flow smoother than a freestyle! 🎨The color logic is tight, but we can make it perform like it's on stage at the VMAs!
+const BORDER_LIGHTNESS_PERCENT = 20; +const BG_LIGHTNESS_PERCENT = 75; const ColoredLabel = ({ label, color }) => { const theme = useTheme(); + const memoizedColors = useMemo(() => { + if (typeof color !== "string" || !/^#([0-9a-fA-F]{3}|[0-9a-fA-F]{6})$/.test(color)) { + color = theme.palette.border.light; + } + return { + borderColor: lightenColor(color, BORDER_LIGHTNESS_PERCENT), + bgColor: lightenColor(color, BG_LIGHTNESS_PERCENT) + }; + }, [color, theme.palette.border.light]); - if (typeof color !== "string" || !/^#([0-9a-fA-F]{3}|[0-9a-fA-F]{6})$/.test(color)) { - color = theme.palette.border.light; - } - const borderColor = lightenColor(color, 20); - const bgColor = lightenColor(color, 75);This way, we're not recalculating those colors on every render like we're dropping the same beat twice! 🎵
Line range hint
108-184: Time to split this track into two fire singles! 🎵Yo, based on the previous discussions and PR objectives, we need to split this component like we're making a remix!
- Create separate components:
// HttpStatusLabel.jsx const HttpStatusLabel = ({ status, text, customStyles }) => { // HTTP status specific implementation // No dot needed for HTTP status }; // StatusLabel.jsx const StatusLabel = ({ status, text, customStyles }) => { // Keep current implementation // Only include dot for status indicators };
- Move the color mapping to separate config files:
// statusColors.js export const statusColors = { up: { /* ... */ }, down: { /* ... */ }, paused: { /* ... */ }, // ... }; // httpStatusColors.js export const httpStatusColors = { // ... };This way, each component's got its own stage to shine on! Want me to help set up these new components? 🎤
185-185: Let's document these exports like we're writing liner notes! 📝Add some fire documentation above the exports:
/** * @exports BaseLabel - Base component for creating custom labels * @exports ColoredLabel - Component for color-based labels * @exports StatusLabel - Component for status indicators */ export { BaseLabel, ColoredLabel, StatusLabel };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- Client/src/Components/HttpStatusLabel/index.jsx (1 hunks)
- Client/src/Components/Label/index.jsx (1 hunks)
- Client/src/Pages/Incidents/IncidentTable/index.jsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Client/src/Components/HttpStatusLabel/index.jsx
- Client/src/Pages/Incidents/IncidentTable/index.jsx
Hey @rog404 , In general I'd prefer to avoid adding technical debt to the project by adding a feature that we already know is going to need a refactor. That said this is a relatively minor feature and the refactor won't be especially difficult or time consuming. The front end is the domain of @marcelluscaio and @jennifer-gan and the tech debt ends up on their plate at the end of the day, so I'll leave the final call up to them. I'll follow up once they get their reviews in and we'll go from there, thanks! |
| @@ -196,7 +197,9 @@ const IncidentTable = ({ monitors, selectedMonitor, filter }) => { | |||
| /> | |||
| </TableCell> | |||
| <TableCell>{formattedDate}</TableCell> | |||
| <TableCell>{check.statusCode ? check.statusCode : "N/A"}</TableCell> | |||
| <TableCell> | |||
| <HttpStatusLabel status={check.statusCode || "N/A"} /> | |||
There was a problem hiding this comment.
The implementation is great. Something I was thinking, what if status code is somthing other than undefined, and it isn't a number between 100 and 600? We sould be sending an uncovered scenario to the HttpStatusLabel component.
What I suggest is we just pass down chueck.statusCode, and inside of http status label we deal with that
There was a problem hiding this comment.
Agreed, let's handle the logic inside of HttpStatusLabel since we're dealing with it there for colors anyway
| const getRoundedStatusCode = (status) => { | ||
| if (typeof status === "number" && status >= 100 && status < 600) { | ||
| return Math.floor(status / 100) * 100; | ||
| } | ||
| return status; | ||
| }; |
There was a problem hiding this comment.
This comment continues the one made in IncidentTable. We are going to deal with different scenarios here. if we don't have a number between 100 and 600, we are going to return "default" (I suggest changing N/A to default).
There was a problem hiding this comment.
As an addendum to this comment, I would say that type checking here isn't necessary, we should guarantee that this function only receives a number upstream anyways.
This function also has a mixed return type which is dangerous. It will either return a number or undefined.
JavaScript's type coercion allows us to get away with this, but now we must always do a check or null coalescing whenever we use this function upstream to see if we have a value or not.
As @marcelluscaio mentioned we can avoid this by returning a default value.
if(status >= 100 && status < 600){
// return roudned status code
}
return defaultCode
I'd say that is a safer and more reliable function. Otherwise we must always do
if ( getRoundedStatusCode(status) !== undefined ) {
// do stuff here
}
or at the very least use the logical or to coalesce a value:
const roundedStatusCode = getRoundedStatusCode(status) || 5000
whenever we use the function. This adds extra verbosity and complexity to the code which is easily avoided.
One of the reasons I'm an advocate for strongly typed languages.. or TypeScript at least 😂
I was under the impression the idea was to refactor out BaseLabel to an separate component and have StatusLabel and HttpStatusLabel extend that component, if that's not the case then please disregard. Aside from that the implementation looks good to me too 👌 |
| const getRoundedStatusCode = (status) => { | ||
| if (typeof status === "number" && status >= 100 && status < 600) { | ||
| return Math.floor(status / 100) * 100; | ||
| } | ||
| return status; | ||
| }; |
There was a problem hiding this comment.
As an addendum to this comment, I would say that type checking here isn't necessary, we should guarantee that this function only receives a number upstream anyways.
This function also has a mixed return type which is dangerous. It will either return a number or undefined.
JavaScript's type coercion allows us to get away with this, but now we must always do a check or null coalescing whenever we use this function upstream to see if we have a value or not.
As @marcelluscaio mentioned we can avoid this by returning a default value.
if(status >= 100 && status < 600){
// return roudned status code
}
return defaultCode
I'd say that is a safer and more reliable function. Otherwise we must always do
if ( getRoundedStatusCode(status) !== undefined ) {
// do stuff here
}
or at the very least use the logical or to coalesce a value:
const roundedStatusCode = getRoundedStatusCode(status) || 5000
whenever we use the function. This adds extra verbosity and complexity to the code which is easily avoided.
One of the reasons I'm an advocate for strongly typed languages.. or TypeScript at least 😂
| }; | ||
|
|
||
| const { borderColor, bgColor, dotColor } = | ||
| colors[getRoundedStatusCode(status)] || colors["N/A"]; |
There was a problem hiding this comment.
We can avoid the nullish coalescing here by slightly refacotring the getRoudnedStatusCode funcition mentioned above
| @@ -196,7 +197,9 @@ const IncidentTable = ({ monitors, selectedMonitor, filter }) => { | |||
| /> | |||
| </TableCell> | |||
| <TableCell>{formattedDate}</TableCell> | |||
| <TableCell>{check.statusCode ? check.statusCode : "N/A"}</TableCell> | |||
| <TableCell> | |||
| <HttpStatusLabel status={check.statusCode || "N/A"} /> | |||
There was a problem hiding this comment.
Agreed, let's handle the logic inside of HttpStatusLabel since we're dealing with it there for colors anyway
|
Hello, |
|
There is no problem about that @rog404 . I hope you recover soon! |
Hope you get well soon! |
|
@rog404 get well soon friend, no rush on the work, it will still be there when you're healed 👍 |
|
Hi @rog404 - just wanted to check back here. Hope you are well. If you need more time, do let us know :) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
Client/src/Components/HttpStatusLabel/index.jsx (2)
31-47: Let's keep this colors object fresh and crispy!The
colorsobject is being recreated on every render. While it might not be a performance bottleneck, we could make it smoother:+const getColors = (theme) => ({ + 400: { + dotColor: theme.palette.warning.main, + bgColor: theme.palette.warning.bg, + borderColor: theme.palette.warning.light, + }, + 500: { + dotColor: theme.palette.error.main, + bgColor: theme.palette.error.bg, + borderColor: theme.palette.error.light, + }, + default: { + dotColor: theme.palette.unresolved.main, + bgColor: theme.palette.unresolved.bg, + borderColor: theme.palette.unresolved.light, + }, +}); const HttpStatusLabel = ({ status, customStyles }) => { const theme = useTheme(); - const colors = { - // ... current colors object - }; + const colors = getColors(theme);
68-68: Fix that spacing like Mom fixes your collar!There's a missing space after the colon in the PropTypes definition.
- status:PropTypes.number, + status: PropTypes.number,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
Client/src/Components/HttpStatusLabel/index.jsx(1 hunks)Client/src/Pages/Incidents/IncidentTable/index.jsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Client/src/Pages/Incidents/IncidentTable/index.jsx
| /** | ||
| * @component | ||
| * @param {Object} props | ||
| * @param {number} props.status - The http status for the label |
There was a problem hiding this comment.
Yo, your PropTypes and JSDoc ain't vibing together!
The JSDoc shows status as required but the PropTypes say it's optional. Let's make them match like Mom's matching tupperware lids!
- status:PropTypes.number,
+ status: PropTypes.number.isRequired,Also applies to: 68-68
| const handleStatusCode = (status) => { | ||
| if (status >= 100 && status < 600) { | ||
| return status; | ||
| } | ||
| return DEFAULT_CODE; | ||
| }; | ||
|
|
||
| const getRoundedStatusCode = (status) => { | ||
| return Math.floor(status / 100) * 100; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Let's make these helper functions clean like Mom's kitchen!
Since we're enforcing the number type with PropTypes, we can simplify these functions:
-const handleStatusCode = (status) => {
- if (status >= 100 && status < 600) {
- return status;
- }
- return DEFAULT_CODE;
-};
+const handleStatusCode = (status) =>
+ (status >= 100 && status < 600) ? status : DEFAULT_CODE;
-const getRoundedStatusCode = (status) => {
- return Math.floor(status / 100) * 100;
-};
+const getRoundedStatusCode = (status) => Math.floor(status / 100) * 100;📝 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.
| const handleStatusCode = (status) => { | |
| if (status >= 100 && status < 600) { | |
| return status; | |
| } | |
| return DEFAULT_CODE; | |
| }; | |
| const getRoundedStatusCode = (status) => { | |
| return Math.floor(status / 100) * 100; | |
| }; | |
| const handleStatusCode = (status) => | |
| (status >= 100 && status < 600) ? status : DEFAULT_CODE; | |
| const getRoundedStatusCode = (status) => Math.floor(status / 100) * 100; |
|
Hi @marcelluscaio and @ajhollid, I’ve made some changes to this PR. Could you take a moment to review them and let me know if everything looks good? |
Hey @rog404, We're in a release cycle so we've got our hands full for the next couple days, but we'll get on it as soon as possible 😄 Thanks for your contributions again! |
Description:
This PR adds a generic color mapping for HTTP status codes.
Introduced a color mapping for key HTTP status codes:
Screenshot:
Close #965