Differential D3409
[web] [refactor] move unread button css to its own component Authored by • benschac on Mar 11 2022, 10:55 AM. Tags None Referenced Files
Details Moving some of the chat-thread-list.css css to chat-thread-list-item-menu.css as a way to separate some of the CSS we're using in multiple places. The goal is to isolate the CSS so we don't break functionality in other parts of the app. At some point down the road, we should break up all of the css files and remove replicated CSS. https://linear.app/comm/issue/ENG-868/break-out-thread-list-css-into-their-own-files make sure mark as unread css is working as expected. const menuIconSize = renderStyle === 'chat' ? 24 : 16;
const btnCls = classNames(css.menuContent, {
[css.menuContentVisible]: menuVisible,
});
return (
<div className={css.menu} onMouseLeave={hideMenu}>
<button onClick={toggleMenu}>
<SWMansionIcon icon="menu-vertical" size={menuIconSize} />
</button>
<div>
<button className={btnCls} onClick={toggleUnreadStatus}>
<SWMansionIcon className={css.mailIcon} icon="mail" size={18} />
{toggleUnreadStatusButtonText}
</button>
</div>
</div>
);
}looking at the mark as unread component, all CSS classes moved here are used in the component.
Diff Detail
Event TimelineComment Actions Can you make your test plan more detailed? There's basically no information being communicated right now. The point of requiring the field is to make you detail the ways that you tested your diff Comment Actions In particular, I'm worried that I don't see you mention anything about checking the code for these CSS classes. You should be running git grep for each of these class names to make sure we're not still using them somewhere that uses chat-thread-list.css. This is an example of what I was talking about in our 1:1 – don't just do simple testing, also rely on thorough analyses of the codebase to make sure your changes don't have side effects. (You didn't detail any "simple testing" in your test plan either, though...) Comment Actions You make a really good point here. The CSS Module`chat-thread-list.css` is used in NINE different places!
These files use some of the classes that are removed, which is pretty scary. I don't think it's worth doing a larger refactor right now. I'm just going to copy-paste the CSS I need in chat-thread-list-item-menu and not remove the CSS in chat-thread-list so we don't have to do a larger refactor right now. Thanks for calling this out. Comment Actions Accepting assuming this is a simple copy-paste. If it's not a simple copy-paste, then please separate out the changes into a different diff, and land this one as a simple copy-paste. Going forward, please ALWAYS think of the impact of your changes across the codebase. You simply cannot be removing any kind of code anywhere without making sure it's not used somewhere. Comment Actions yes, I didn't change any of the code, just moved it.
Yep, I should have done this beforehand. Especially, after having to split out CSS in other parts of the code base. |