Skip to content

Conversation

bojidar-bg
Copy link
Contributor

@bojidar-bg bojidar-bg commented Jul 6, 2025

Stores the sidebar state per-room in LegacyCallHandler, where other details about calls are already stored.

Fixes #25367.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

Stores the sidebar state per-room in LegacyCallHandler, along with other details about calls.
@bojidar-bg bojidar-bg requested a review from a team as a code owner July 6, 2025 12:04
@bojidar-bg bojidar-bg requested review from dbkr and florianduros July 6, 2025 12:04
@CLAassistant
Copy link

CLAassistant commented Jul 6, 2025

CLA assistant check
All committers have signed the CLA.

@dbkr
Copy link
Member

dbkr commented Jul 7, 2025

Thanks for this! The code looks straightforward enough. Just one minor lint error. A new feature would need test coverage though.

@bojidar-bg
Copy link
Contributor Author

Huh. I'm this sure I ran the linter, guess I didn't 😂

Should I make the test in test/unit-tests/components/views/voip/LegacyCallView-test.tsx? Or would something around playwright/e2e/voip be advisable? Just making sure, as I didn't see other tests covering the rest of LegacyCallView's features to go off of (:

The toggle sidebar button currently does nothing in PIP mode, since PIP mode never shows a sidebar (even when the call is made fullscreen from the PIP preview)
@bojidar-bg bojidar-bg force-pushed the 25367-remember-call-sidebar branch from f38f28c to dd0d27b Compare July 7, 2025 17:24
@bojidar-bg
Copy link
Contributor Author

BTW, this PR was made as part of a live stream/speedrun (: You can see the final demonstration of the feature working at 2:10:28 - and yes, I did run the linter at 1:29:16, but then I never ran it on the PIP changes 😂
(just a shamless plug 😇)

@dbkr
Copy link
Member

dbkr commented Jul 8, 2025

Oh, nice! I'd probably do a jest test in LegacyCallView-test.tsx as it should be quite simple to assert that the dom element is there. There's probably no need to make it a playwright test.

Also reminder not to force push PRs please, it means we lose the record of what code has been reviewed.

@bojidar-bg
Copy link
Contributor Author

Alright, added a test (:

@MidhunSureshR
Copy link
Member

👋, gentle reminder that we can't merge this PR until we meet the test coverage requirements. Let me know if you're unable to write more tests.

Also, fix issue where LegacyCallViewForRoom used roomId and not callId for checking for sidebar state
@bojidar-bg
Copy link
Contributor Author

bojidar-bg commented Aug 25, 2025

Apologies for the delay - had a few busy weeks 😅

Added a bunch of tests, hopefully this would be enough to pass the coverage check?

(PS, just a tiny contributor experience nitpick: as a new contributor, I never stumbled across the Review guidelines file -- so both the reminder to not force-push and the reminder that coverage check is required came as surprises, and not as reminders. (Granted, coverage check is mentioned in CONTRIBUTING.md, but it's a tiny, easy-to-miss line in a sea of text - and it's not spelled out as part of the pull request checklist with the other requirements.))

@dbkr dbkr added this pull request to the merge queue Sep 1, 2025
Merged via the queue into element-hq:develop with commit 8903927 Sep 1, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remember sidebar status for an ongoing call
4 participants