Experienced Devs: How to handle code review pushback and non-compliant devs

Context: I got a job recently and was asked to embed with another in-house team for a few weeks until my team came back from vacation/parental leave. There isn’t another dedicated frontend engineer on this team but several generalist engineers with limited front end experience. One engineer in particular who seems relatively junior (full disclosure, I have ~6 YOE), works primarily on front end.When I joined the team, I noticed a number of issues throughout the codebase but no serious showstoppers. During code review (and via some git blame investigations), I noticed the majority of these problems came from this junior engineer, so I paid a bit closer attention to their PRs when doing code review. As a general rule, I try to keep my review comments concise and focused on the code, not the person and try to add positive comments as well when I think something was designed or implemented well.I noticed this dev had a habit of “running out the clock” on their work. They would ship something within a reasonable timeframe and move onto the next ticket before getting approval on the first ticket. Then they would apply pressure in stand up to just “ship it” since “it works fine.” Often, I had to just approve things to get it out the door. Previously, the other engineers would just rubber stamp all front end PRs.Examples of critique (tweaked to omit details)”This internal component is redefined on each render. Doesn’t look like it has any dependencies on the outer scope so it can moved out entirely.””Add a role and aria label for these icons so they can be read by screen readers””This test contains a lot of implementation detail. It’s a lot simpler to assert that the button text matches what we expect when the number of the items in the cart is greater than 10. Otherwise, this test will break whenever we refactor unrelated logic. Also, let’s add an integration test so we can be sure that clicking on the button will generate the checkout modal.””string | any will always evaluate to any, so we’re losing type safety here. Is this an intentional change?””This component is calling the api endpoint directly. Let’s create a new method on the client and add this logic so we can call it in a more generic way like client.getUserPreferences()”At the end of my stay, I participated in an “unofficial” review and was surprised to receive negative feedback from said developer citing these comments. This negative feedback stated I was too nitpicky and micromanaged their work, which frustrated them because they expended more effort and time refactoring than they did writing code, which lead to a delay in completing their tickets.The team lead asked how I could improve my technical communication moving forward but I was at a loss to explain how, given that I thought these comments weren’t out of line.I’m not sure what I did wrong here. I think my comments are being mischaracterized and my reviews were valid. Almost all of these comments would only require some minor changes? via /r/Frontend https://ift.tt/3kDANf5