I was recently tasked with rewriting the base CSS for an inventory/project management system, creating a set of reusable components designed to match, using an open/close approach. These were based on a pretty strict specification provided by one of our designers, who unfortunately left.
The implementation went well, but I’ve run into a bit of a problem. Quite often the team members make changes directly to the base class in the new base CSS file, rather than extending it, creating a new one, or using each system area’s dedicated stylesheet file.
One of the more recent changes involved removing a grid-gap property from a rule from the base CSS, affecting a lot more than the single UI element the team member was working on.
Should I approach the team about this?
I haven’t mentioned anything yet, but have noticed our QA team putting in more bugs about UI elements looking odd
That sounds like a pretty straight forward and simple conversation.
Do you do code reviews? Have code owners? If you (or a set of people you trust) were required to sign off on changes on the base files that aren’t supposed to be changed willy-nilly, you could catch it before it went to main
I agree, code review is the proper solution here
It’s the best way to propagate coding culture.
Step 1: Get buy in. Discuss with the team and agree that it’s a good idea. Write it down so there’s a paper trail. Link to it in important base files in comments as a reminder as to what the guidelines are.
Step 2: Code review. Make sure the right people are required on code reviews for those base files. As the issues are brought up and fixed it will become a cultural habit and self propagate.
This sounds like a job for a team wide code review process.
If you don’t say anything, it won’t get better. Up to you whether that’s worth the hassle, based on your team and your situation, of course
Should I approach the team about this?
Yes, certainly. It sounds like some/most members of the team don’t understand the stylesheet architecture of your project and it’s vital to sort that out ASAP. The more time passes, the harder it will get to get everything back on track.
Try to avoid finger-pointing and this shouldn’t be a very hard conversation (assuming they aren’t super stubborn).
As a side note, the fact that you first address this issue on the Fediverse and not in your team makes me think that maybe there are some underlying issues regarding trust and open communication. You might want to further look into that. Take it with a large grain of salt though. I don’t really know anything about your team so it’s likely that I overdramatize the situation.
Someone in your organization needs to be in charge of frontend fidelity. I don’t mean an official job title, I just mean someone who has taken it upon themselves to have a “the buck stops here” mentality—better yet someone who is recognized by the rest of the team to have that priority.
If nobody else fits the bill, then that person is you. And by all means, make all the stink you want about these issues. Nobody should ever be touching global stylesheets that affect multiple components or screens throughout the system without there being subsequent review or issues filed in a very visible way. Ideally, those sorts of breaking changes would never make it through code review in the first place.
-
passive aggressive email
-
let qa bust them
-
collective email reminder to the whole team not to edit the base class, variation of #1
-
fix it yourself and say nothing, snitches get stitches
-
snitch
-
create a copy of the base further down so it just overwrites their changes – I forget if this works, maybe make a copy above too just to cover your angles
-
ask them to find the bugs in someone else’s commits and see if they fix their own shit
- create a copy of the base further down so it just overwrites their changes
lol.
Also, I guess it’s a variant of #7: Tell them that their code has caused bugs in existing code and ask them to fix it.
You seem to have a specific set of goals in mind. Please share what those goals are.
Oh, this is like a buffet. Pick and choose. Don’t worry about the conditions back in the kitchen or how we passed our last inspection by wheeling all the expired food out to the dumpster temporarily.
-
Do you have code review?
We do, however they’re usually assigned to devs that are not familiar with the work carried out, thus these kinds of things end up slipping into the main branch…
Looking at the other responses though this is something that I’ll need to raise with the team, some here have pointed out some pretty insightful stuff.
You may want to review your process then to make it clearer who should be on what review for different parts of the code base. You could use team groups to define who should be required for who.
I think this can be resolved with code review (as others have mentioned), but there’s also a lack of understanding on how to use the components you have created. Maybe a quick email politely explaining the theory/approach behind these base classes, with some examples of do-s and don’t-s could work alongside code review to enforce the change
You can keep bringing it up. But, in my exp, if you dont have anyone high up that can support your perspective, it’s just gonna be an uphill battle. And is likely just going to make the other devs unhappy with you.
It’s unfortunate but most devs dont really like anything that’s going to cause them more work (e.g. more code reviews, higher quality changes, looking at the bigger impact of their changes etc).
If you don’t have someone higher up —maybe the manager of the managers of those problematic engineers—I’d just make more tests around the areas that are breaking and require those tests to pass before merging code changes. Devs may not like to work harder, but they damn sure dont like seeing a bunch of red X’s when they open a PR lol 😃
