[web] [refactor] move unread button css to its own component
ClosedPublic
Actions

Authored by • benschac on Mar 11 2022, 10:55 AM.

Details

Reviewers

atul
ashoat

Commits

rCOMM465115e6960f: [web] [refactor] move unread button css to its own component

Summary

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

Test Plan

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

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• benschac created this revision.Mar 11 2022, 10:55 AM

Herald added subscribers: • karol, • adrian, tomek, ashoat. · View Herald TranscriptMar 11 2022, 10:55 AM

Harbormaster completed remote builds in B7310: Diff 10310.Mar 11 2022, 10:59 AM

• benschac requested review of this revision.Mar 11 2022, 10:59 AM

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

This revision now requires changes to proceed.Mar 12 2022, 11:02 AM

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...)

In D3409#92233, @ashoat wrote:

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...)

You make a really good point here. The CSS Module`chat-thread-list.css` is used in NINE different places!

chat-thread-list-see-more-sidebars.react
chat-thread-list-side-bar.react
sidebar-list-modal.react

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.

remove delete from chat-thread-list.css

Harbormaster completed remote builds in B7336: Diff 10337.Mar 14 2022, 9:08 AM

• benschac edited the summary of this revision. (Show Details)Mar 14 2022, 9:13 AM

• benschac edited the test plan for this revision. (Show Details)

• benschac edited the test plan for this revision. (Show Details)Mar 14 2022, 9:15 AM

• benschac added a child revision: D3421: [web] [fix] hover state background color on thread list.Mar 14 2022, 9:42 AM

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.

This revision is now accepted and ready to land.Mar 14 2022, 9:16 PM

In D3409#92731, @ashoat wrote:

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.

yes, I didn't change any of the code, just moved it.

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.

Yep, I should have done this beforehand. Especially, after having to split out CSS in other parts of the code base.

Closed by commit rCOMM465115e6960f: [web] [refactor] move unread button css to its own component (authored by • benschac). · Explain WhyMar 15 2022, 6:43 AM

This revision was automatically updated to reflect the committed changes.

• benschac added a commit: rCOMM465115e6960f: [web] [refactor] move unread button css to its own component.