Details

Reviewers

atul
ginsu
tomek

Commits

rCOMMb18a354ea61d: [web] Display the number of pinned messages in a banner across chat

Summary

Each thread that has pinned messages should display a banner across the top indicating the count.

Linear: https://linear.app/comm/issue/ENG-3448/display-the-number-of-pinned-messages-in-a-banner-across-chat

Depends on D7309

Test Plan

Check threads that have 1) 0 pinned messages, 2) 1 pinned messages, 3) >1 pinned messages

Screenshot 2023-04-04 at 10.31.28 PM.png (1×3 px, 351 KB)

Screenshot 2023-04-04 at 10.31.19 PM.png (1×3 px, 325 KB)

Screenshot 2023-04-04 at 10.31.13 PM.png (1×3 px, 371 KB)

Diff Detail

Repository

rCOMM Comm

Branch

pinned_messages

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Apr 4 2023, 7:35 PM

Herald added a subscriber: ashoat. · View Herald TranscriptApr 4 2023, 7:35 PM

rohan edited the test plan for this revision. (Show Details)Apr 4 2023, 7:36 PM

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

rohan attached a referenced file: F463445: Screenshot 2023-04-04 at 10.31.19 PM.png. (Show Details)

rohan attached a referenced file: F463447: Screenshot 2023-04-04 at 10.31.13 PM.png. (Show Details)

rohan attached a referenced file: F463446: Screenshot 2023-04-04 at 10.31.28 PM.png. (Show Details)

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 4 2023, 7:50 PM

Harbormaster failed remote builds in B17994: Diff 24650!

Fix eslint warnings

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 5 2023, 5:51 AM

Harbormaster failed remote builds in B17997: Diff 24655!

Rebase + update diff

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 11 2023, 5:27 AM

Harbormaster failed remote builds in B18224: Diff 24960!

I still need to figure out what is causing tooltip-utils.test.js to fail in D7309, but after attempting to debug locally, I'd prefer to unblock review and continuing figuring it out locally

rohan requested review of this revision.Apr 11 2023, 5:30 AM

rohan added a child revision: D7380: [web] Display the pinned messages for the thread when the banner is clicked.Apr 11 2023, 5:48 AM

tomek accepted this revision.Apr 12 2023, 4:53 AM

tomek added inline comments.

web/chat/thread-top-bar.css
52 ↗	(On Diff #24960)	Please use a const from `typography`
57–60 ↗	(On Diff #24960)	Adding a hover effect to span might indicate that it is not really a span? If clicking it navigates to something, then it is rather an anchor or a button.
web/chat/thread-top-bar.react.js
43 ↗	(On Diff #24960)	How about using string template literal?

This revision is now accepted and ready to land.Apr 12 2023, 4:53 AM

tomek mentioned this in D7380: [web] Display the pinned messages for the thread when the banner is clicked.Apr 12 2023, 5:34 AM

rohan added inline comments.Apr 12 2023, 11:28 PM

web/chat/thread-top-bar.react.js
43 ↗	(On Diff #24960)	I've made some changes to this diff in the upcoming revision, so please let me know if it addresses your feedback or if you feel like I missed over something

ESLint + Address Feedback

Harbormaster failed remote builds in B18296: Diff 25074!Apr 12 2023, 11:57 PM

Still seeing ESLint issues. Might be good to make sure you're able to confirm locally that all ESLint issues are resolved before updating the diff again

In D7310#220751, @ashoat wrote:

Still seeing ESLint issues. Might be good to make sure you're able to confirm locally that all ESLint issues are resolved before updating the diff again

For me, the BuildKite log shows that eslint . --max-warnings=0 is Done in 50.79s, and when I run eslint . locally to check the entire project repo, it doesn't identify any failures.

yarn workspace web test however is what I'm seeing as the cause to the failure, with a message of

$ jest
 PASS  database/sql-js.test.js
 PASS  utils/device-id.test.js
 PASS  utils/text-utils.test.js
 PASS  olm/olm.test.js
 PASS  media/aes-crypto-utils.test.js
 PASS  database/utils/worker-crypto-utlis.test.js
 PASS  database/queries/metadata-queries.test.js
 PASS  database/utils/db-utils.test.js
 PASS  utils/typeahead-utils.test.js
 PASS  database/queries/storage-engine-queries.test.js
 PASS  database/queries/draft-queries.test.js
 FAIL  utils/tooltip-utils.test.js
  ● Test suite failed to run
 
    TypeError: Cannot read properties of undefined (reading 'LEFT')
 
      23 |
      24 | const availableTooltipPositionsForViewerMessage = [
    > 25 |   tooltipPositions.LEFT,
         |                    ^
      26 |   tooltipPositions.LEFT_BOTTOM,
      27 |   tooltipPositions.LEFT_TOP,
      28 |   tooltipPositions.RIGHT,
 
      at Object.<anonymous> (chat/composed-message.react.js:25:20)
      at Object.<anonymous> (chat/multimedia-message.react.js:11:1)
      at Object.<anonymous> (chat/message.react.js:12:1)
      at Object.<anonymous> (components/pinned-message.react.js:12:1)
      at Object.<anonymous> (modals/chat/toggle-pin-modal.react.js:20:1)
      at Object.<anonymous> (utils/tooltip-utils.js:35:1)
      at Object.<anonymous> (utils/tooltip-utils.test.js:3:1)
 
Test Suites: 1 failed, 11 passed, 12 total
Tests:       55 passed, 55 total
Snapshots:   0 total
Time:        4.41 s