Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Extract place call logic to its own file #11494

Merged
merged 5 commits into from
Aug 31, 2023

Conversation

germain-gg
Copy link
Contributor

@germain-gg germain-gg commented Aug 30, 2023

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

@germain-gg germain-gg added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Aug 30, 2023
@germain-gg germain-gg marked this pull request as ready for review August 30, 2023 15:17
@germain-gg germain-gg requested a review from a team as a code owner August 30, 2023 15:17
@germain-gg germain-gg enabled auto-merge August 30, 2023 17:25
Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works 👍

Some minor comments on the code.

@@ -150,7 +126,7 @@ export default function RoomHeader({ room }: { room: Room }): JSX.Element {
disabled={!!voiceCallDisabledReason}
title={!voiceCallDisabledReason ? _t("Voice call") : voiceCallDisabledReason!}
onClick={async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just spotted the async here. The function is not async and it could be removed.

// component
// break;
}
const makeCall = useCallback(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function can probably be removed and placeCall can be called directly.
I can't see any value in it except that it raises the complexity here.

@germain-gg germain-gg requested a review from weeman1337 August 31, 2023 10:32
Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 🙂

@germain-gg germain-gg added this pull request to the merge queue Aug 31, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Aug 31, 2023
@germain-gg germain-gg added this pull request to the merge queue Aug 31, 2023
Merged via the queue into develop with commit 77a468f Aug 31, 2023
19 checks passed
@germain-gg germain-gg deleted the germain-gg/place-call-function branch August 31, 2023 14:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants