Conversation List Highlighting #7453
Conversation
Co-authored-by: trevor-signal <[email protected]>
Co-authored-by: Wyatt Childers <[email protected]>
There was a problem hiding this comment.
@brianHarder thanks for this PR! It looks great; I have one clarifying question.
It's great that you added a mock-test, but I think it's unnecessary for this kind of small stylistic change -- mock tests are great but also have overhead so we don't add them for every change.
| onClick={onClick} | ||
| onMouseDown={onMouseDown} | ||
| onMouseDown={event => { | ||
| event.preventDefault(); |
There was a problem hiding this comment.
can you say more why this change is necessary?
There was a problem hiding this comment.
@trevor-signal thank you for the feedback! I included this line of code because the default Chromium behavior would maintain an 'active' state on a chat list item throughout the drag, which I believe could cause the highlight to linger. This change was specifically to address the highlight during dragging.
There was a problem hiding this comment.
Can you say more? If I remove this line, things seem to continue to work well.
| @@ -0,0 +1,99 @@ | |||
| // Copyright 2024 Signal Messenger, LLC | |||
There was a problem hiding this comment.
@brianHarder like I mentioned in the review, this is really cool that we could have a mock test here, but I think a mock test for this minor UI change is unnecessary (each mock test has an overhead). Do you mind (sadly) removing it?
stylesheets/_modules.scss
Outdated
|
|
||
| @include mixins.keyboard-mode { | ||
| &:focus:not(:disabled, &--disabled, &--is-selected) { | ||
| @include mixins.light-theme { |
There was a problem hiding this comment.
Why are we trying to set the colors to the opposite of the hover state above (I may just be not understanding).
Also, have you tested this in light mode? I'm not sure nested mixings work the way we always intend. We've been moving to the newer CSS syntax light-dark() which might work well here.
|
@trevor-signal I believe I've addressed all outstanding feedback! Here are the changes:
SignalPr4fixes.mov |
|
Thanks, @brianHarder! We've merged this internally and it will be released with our next beta release. |
Co-authored-by: trevor-signal <[email protected]> Co-authored-by: Brian Harder <[email protected]>
Co-authored-by: Brian Harder <[email protected]>
Contributor checklist:
mainbranchpnpm run readyrun passes successfully (more about tests here)Description
This PR ensures that highlighting in the chat list only occurs by mouse hover and keyboard navigation. Previously there was an issue where clicking and dragging on a chat would cause it to stay highlighted both throughout the drag and even after mouse release. To achieve this, the default mouse behavior has been prevented, and new CSS classes have been added to only highlight conversation lists via mouse hover or the tab key.
I manually tested this PR by performing mouse movements in my staging environment and ensuring that the highlighting was either applied or not applied as expected. I specifically simulated the click and drag several times to make sure the highlight was not lingering. I also added a test file - conversationListHighlight_test.ts - which simulates a hover movement to check for highlighting and a click-drag movement to check that the highlight does not stay. I did all my testing on my laptop, which runs macOS 10.15.7.
Screen.Recording.2025-08-15.at.3.59.53.PM.mov