Page MenuHomePhabricator

[web] [refactor] last message color style
AbandonedPublic

Authored by benschac on Mar 21 2022, 12:53 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 30, 11:09 AM
Unknown Object (File)
Sat, Nov 9, 10:59 AM
Unknown Object (File)
Sat, Nov 9, 10:46 AM
Unknown Object (File)
Sat, Nov 9, 7:12 AM
Unknown Object (File)
Tue, Nov 5, 12:02 PM
Unknown Object (File)
Oct 26 2024, 1:08 PM
Unknown Object (File)
Oct 11 2024, 4:31 PM
Unknown Object (File)
Oct 11 2024, 3:48 PM

Details

Summary

move repeated styles into top of the function.

Image 2022-03-21 at 3.55.51 PM.jpg (762×938 px, 52 KB)

Note: unread time padding needs to be fixed. I will do it in another diff.

Test Plan

send a couple of messages to yourself to make sure that read/unread message preview still work as expected.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

benschac edited the test plan for this revision. (Show Details)
benschac added reviewers: jacek, tomek.
benschac edited the summary of this revision. (Show Details)
tomek requested changes to this revision.Mar 22 2022, 4:37 AM

I'm not sure if defining all the styles at the top is the best idea - they should be defined closer to where they are used (before if (messageInfo.type === messageTypes.TEXT) {).

But there's more important issue here: if we should simplify this code even further. In one path we render

<div className={lastMsgCls}>
        {usernameText}
        {messageTitle}
      </div>

which differs by only usernameText from the last return. This is a great opportunity for simplification, because let usernameText = null; can be moved outside the if which will result in return being called after the if. That will also affect where the styles should be defined.

This diff already refactors and fixes the code, so I'm going to request changes. We can refactor and fix in the same diff, but finding a way to split it will be really appreciated.

web/chat/message-preview.react.js
42–62
This revision now requires changes to proceed.Mar 22 2022, 4:37 AM

Closing this diff in favor of my refactor stack.