this post was submitted on 05 Apr 2024
217 points (98.2% liked)
Programmer Humor
32453 readers
317 users here now
Post funny things about programming here! (Or just rant about your favourite programming language.)
Rules:
- Posts must be relevant to programming, programmers, or computer science.
- No NSFW content.
- Jokes must be in good taste. No hate speech, bigotry, etc.
founded 5 years ago
MODERATORS
you are viewing a single comment's thread
view the rest of the comments
view the rest of the comments
I’m Hedgehog, the poor senior dev who was assigned to review Hal’s code.
Panel 1: ✅ (PR Approved) LGTM but you’re missing the styling from the mock-ups, should be easy to add.
Panel 2: ❌ (Changes requested)
Nit: Hal, your PR failed in CI. You should have used
const
instead oflet
. Did you forget to run the linter before pushing?Also, the
useState
hook isn’t doing anything. If it doesn’t need to, just leave it as an uncontrolled component. I didn’t look at the surrounding code but this is part of a form, right? If not then it should be receiving the setter/value as props.Panel 3: ✅ LGTM, ship it.
❌ Actually wait, you still have that do-nothing state code in there. Either get rid of it or do something with it.
Panel 4: ❌ Hal, I don’t like where this is going.
Panel 5: (during stand-up) I reviewed Hal's PR and just had a couple pieces of feedback. Shouldn’t take long, right, Hal?
Panel 6: ❌ WTF, Hal.
<InputField />
is literally just passing through props toinput
, so you don’t need it.Also, Hal, I recommend you look into the Styled Components library. It might better fit your needs here. You could rewrite the LoginComponent as a styled input. Of course, if you do that you should refactor the existing places where you’re using style sheets to use styled components and themes instead.
You also still have the do-nothing
useState
hook for some reason. Seriously, Hal, get rid of it.This is how I’d write this without bringing in Styled Components, but if you use it make sure to test it first: