Use daily-react-hooks in examples by harshithpabbati · Pull Request #72 · daily-demos/examples

harshithpabbati · 2022-03-10T13:07:13Z

This PR is to use the @daily-co/daily-react-hooks package in the examples repo.

vercel · 2022-03-10T13:07:16Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

custom-basic-call – ./custom/basic-call

🔍 Inspect: https://vercel.com/daily-co/custom-basic-call/3HszgbCP8vQfiJagf1bzxfGm4ygj
✅ Preview: https://custom-basic-call-git-daily-react-hooks-daily-co.vercel.app

custom-flying-emojis – ./custom/flying-emojis

🔍 Inspect: https://vercel.com/daily-co/custom-flying-emojis/3RijBRbUxn7ztXXnZpBRoT5DGJJE
✅ Preview: https://custom-flying-emojis-git-daily-react-hooks-daily-co.vercel.app

custom-pagination – ./custom/pagination

🔍 Inspect: https://vercel.com/daily-co/custom-pagination/FuobYjb5XCoKRzLjzCoKAey1bjEp
✅ Preview: https://custom-pagination-git-daily-react-hooks-daily-co.vercel.app

custom-text-chat – ./custom/text-chat

🔍 Inspect: https://vercel.com/daily-co/custom-text-chat/26WNNidG8nMQWiDaYWB2EJQHUyv2
✅ Preview: https://custom-text-chat-git-daily-react-hooks-daily-co.vercel.app

custom-live-streaming – ./custom/live-streaming

🔍 Inspect: https://vercel.com/daily-co/custom-live-streaming/Da5dAVKhcBXkrPrJUyzpqDc6BsyN
✅ Preview: https://custom-live-streaming-git-daily-react-hooks-daily-co.vercel.app

prebuilt-basic-embed – ./prebuilt/basic-embed

🔍 Inspect: https://vercel.com/daily-co/prebuilt-basic-embed/AJ1f5yWPe7icZPDc8XYT452gHB3M
✅ Preview: https://prebuilt-basic-embed-git-daily-react-hooks-daily-co.vercel.app

examples – ./custom/live-transcription

🔍 Inspect: https://vercel.com/daily-co/examples/Cw12A6QHByvjbWNiW1UUbkRTLVAR
✅ Preview: https://examples-git-daily-react-hooks-daily-co.vercel.app

custom-live-transcription – ./custom/live-transcription

🔍 Inspect: https://vercel.com/daily-co/custom-live-transcription/Dn4rNduTbXC11HqqR3XnjsSRHeRE
✅ Preview: https://custom-live-transcription-git-daily-react-hooks-daily-co.vercel.app

custom-active-speaker – ./custom/active-speaker

🔍 Inspect: https://vercel.com/daily-co/custom-active-speaker/5NBeSFj4ojrZ7cs9e1JE1eTSF8n6
✅ Preview: https://custom-active-speaker-git-daily-react-hooks-daily-co.vercel.app

fitness – ./custom/fitness-demo

🔍 Inspect: https://vercel.com/daily-co/fitness/BiN1H2iecwZTzftLZBn898dCF3zT
✅ Preview: https://fitness-git-daily-react-hooks-daily-co.vercel.app

ghost

Just made a small start, adding some initial comments!

ghost · 2022-03-17T15:37:53Z

-    setRatio(rect.width / rect.height);
-    setScreenHeight(rect.height);
+    if (!screenRef.current) return;
+    const { height, width } = screenRef.current.getBoundingClientRect();


Just to confirm, will this work as expected if the aspect ratio is <1?

ghost · 2022-03-17T15:41:51Z

-    }
-    if (hasScreenshares && otherParticipants.length > 0) {
-      items.push(otherParticipants[0]);
+    items.push(localParticipant?.session_id);


This seems like it would potentially push undefined to the array, do we want that?

ghost · 2022-03-17T15:57:31Z

+
+  useResize(() => {
+    const { width, height } = viewRef.current?.getBoundingClientRect();
+    setDimensions({


Seems like the dimensions here can be undefined, what would happen in that case?

probably good to add
if (!width || !height) return;

ghost · 2022-03-17T16:04:28Z

-            height: containerRef.current?.clientHeight,
-          })
-        );
+            width: Math.floor(dims.width),


I think there is a chance that this will result in an unhandled TypeError, since dims can be undefined above. Should we handle that case before trying to use dims.width and dims.height here?

ghost · 2022-03-17T17:19:21Z

+        );
+        const rowCount = Math.ceil(tileCount / columnCount);
+        const rowGap = rowCount - 1;
+        if (rowCount * maxHeightPerTile + rowGap > height) {


Also just confirming: are we expecting to only be adding one count of rowGap here? Or is the rowGap intended to be per tile? (ie should this be rowCount * (maxHeightPerTile + rowGap)?

jayne-mast

Haven't gone through the full thing yet, but here are just a few minor comments. I think in the future it would be better to split this up into multiple PRs

jayne-mast · 2022-03-17T09:19:38Z

+        {!isOwner ? (
+          <>
+            <span
+              className={!participant.video ? 'state error' : 'state success'}


In general, negation is harder to parse, so I would suggest rewriting this and the other ternary operators to avoid it

Suggested change

className={!participant.video ? 'state error' : 'state success'}

className={participant.video ? 'state success' : 'state error'}

jayne-mast · 2022-03-17T09:24:33Z

-import { useCallState } from '@custom/shared/contexts/CallProvider';
+import React, { createContext, useContext } from 'react';
 import { useUIState } from '@custom/shared/contexts/UIStateProvider';
+import { useLiveStreaming as useDailyLiveStreaming } from '@daily-co/daily-react-hooks';


Why did you rename the variable here?

jayne-mast · 2022-03-17T09:25:22Z


 const MIN_TILE_WIDTH = 280;
-const MAX_TILES_PER_PAGE = 12;
+const MAX_TILES_PER_PAGE = 25;


Why is this changed?

jayne-mast · 2022-03-17T16:05:56Z

+    if (disableAudio) return null;
+    if (isSafari() || topology === 'peer') {
+      return subscribedIds.map((sessionId) => (
+        // @ts-ignore


Why are we doing a ts-ignore here?

jayne-mast · 2022-03-17T16:07:10Z

+  useEffect(() => {
+    if (!audioEl.current) return;
+    const AC = AudioContext || webkitAudioContext;
+    audioCtx.current = window.audioContext ?? new AC();


When is window.audioContext null and not undefined?

ghost · 2022-03-18T14:24:46Z

+  return (
+    <div className="person-row">
+      <div className="name">
+        {participant.user_name} {participant.local && '(You)'}


I think the returned participant can be null, should probably check for that

ghost · 2022-03-18T14:30:47Z

+
+  return (
+    <option value={participant.session_id} key={sessionId}>
+      {participant.user_name}


same here, it might be worth adding a null check since useParticipant() can return null

ghost · 2022-03-18T14:32:15Z

+    };
+
+    if (layoutType === 'single-participant')
+      config.layout.session_id = participantId;


Nit: I get that sometimes single-line conditionals without braces read smoother, but imo multiline conditionals usually read easier and more consistently with braces around them.

ghost · 2022-03-18T14:35:51Z

+        // Get the sender's current display name or the local name
+        const sender =
+          e.data?.session_id !== participants.local.session_id
+            ? participants[e.data.session_id].user_name


Is there any chance of e.data being undefined or null when we get here (since we have optional chaining for it right above) and running into a TypeError while trying to access it on this line?

ghost · 2022-03-18T14:40:31Z

-      swapParticipantPosition(sortedVisibleRemoteParticipants[0].id, peerId);
-    },
-    [page, swapParticipantPosition, visibleParticipants]
+  const sortedVisibleRemoteParticipantIds = useDeepCompareMemo(


I am guessing it doesn't make sense to use/showcase the hooks useParticipantIds() sort parameter here?

ghost · 2022-03-18T14:43:50Z

@@ -27,32 +31,34 @@ export const RECORDING_IDLE = 'idle';

 export const RECORDING_TYPE_CLOUD = 'cloud';
 export const RECORDING_TYPE_CLOUD_BETA = 'cloud-beta';


I am not sure that the cloud-beta recording type is a thing anymore, do you know? The docs specify cloud, local, output-byte-stream, and rtp-tracks

correct - cloud is now what was previously referred to as cloud-beta

jessmitch42 · 2022-03-23T21:02:13Z

+  const { aspectRatio, height, maxWidth } = useMemo(() => {
+    /**
+     * We're relying on calculating what there is room for
+     * the total number of s+p tiles instead of using


let's use the full word instead of s+p (can't think of what that stands for)

jessmitch42 · 2022-03-23T21:03:22Z

  }, [updateRatio]);

-  useEffect(() => updateRatio(), [updateRatio]);
-


why remove this? it looks like it needs to be called on render from the looks of it but maybe it's unnecessary?

jessmitch42 · 2022-03-23T21:04:04Z

    <Tile
      aspectRatio={finalRatio}
-      participant={participant}
+      sessionId={sessionId}


are these changes only converting to daily hooks or are there additional changes?

jessmitch42 · 2022-03-23T21:09:37Z


 export const SpeakerView = () => {
-  const { currentSpeaker, localParticipant, participants, screens } =
+  const { currentSpeakerId, localParticipant, screens, orderedParticipantIds } =


To me it's confusing to use localParticipant from the useParticipants provider still. Why not the Daily Hook? Is there a use case where we would need it from the participants provider? If not, it can be removed

jessmitch42 · 2022-03-23T21:16:50Z

-      onJoin(roomName, owner, fetchToken);
-    }}>
+    <form
+      onSubmit={(e) => {


this can be moved into submitForm named method. (Please avoid inline functions as much as possible as a general rule👍 )

rememberlenny · 2022-03-29T21:01:25Z

Looking forward to this!

harshithpabbati marked this pull request as draft March 10, 2022 13:07

vercel Bot deployed to Preview – custom-basic-call March 11, 2022 11:57 View deployment

vercel Bot deployed to Preview – custom-text-chat March 11, 2022 11:57 View deployment

vercel Bot deployed to Preview – prebuilt-basic-embed March 11, 2022 11:57 View deployment

vercel Bot deployed to Preview – custom-live-streaming March 11, 2022 11:57 View deployment

vercel Bot deployed to Preview – custom-flying-emojis March 11, 2022 11:57 View deployment

vercel Bot deployed to Preview – examples March 11, 2022 11:57 View deployment

vercel Bot deployed to Preview – custom-active-speaker March 11, 2022 11:57 View deployment

vercel Bot deployed to Preview – custom-live-transcription March 11, 2022 11:57 View deployment

vercel Bot deployed to Preview – fitness March 11, 2022 11:57 View deployment

vercel Bot deployed to Preview – custom-pagination March 11, 2022 11:57 View deployment

harshithpabbati force-pushed the daily-react-hooks branch from 51a5e2b to 4a55eaf Compare March 11, 2022 12:07

vercel Bot deployed to Preview – custom-basic-call March 11, 2022 12:07 View deployment

vercel Bot deployed to Preview – fitness March 11, 2022 12:07 View deployment

vercel Bot deployed to Preview – custom-live-transcription March 11, 2022 12:07 View deployment

vercel Bot deployed to Preview – prebuilt-basic-embed March 11, 2022 12:07 View deployment

vercel Bot deployed to Preview – custom-flying-emojis March 11, 2022 12:07 View deployment

vercel Bot deployed to Preview – custom-text-chat March 11, 2022 12:07 View deployment

vercel Bot deployed to Preview – custom-pagination March 11, 2022 12:07 View deployment

vercel Bot deployed to Preview – examples March 11, 2022 12:07 View deployment

vercel Bot deployed to Preview – custom-live-streaming March 11, 2022 12:07 View deployment

vercel Bot deployed to Preview – custom-active-speaker March 11, 2022 12:07 View deployment

Use daily-react-hooks in examples

d68ab23

harshithpabbati force-pushed the daily-react-hooks branch from 4a55eaf to d68ab23 Compare March 11, 2022 12:07

harshithpabbati requested review from jayne-mast and jessmitch42 March 11, 2022 12:07

vercel Bot deployed to Preview – custom-basic-call March 11, 2022 12:08 View deployment

vercel Bot deployed to Preview – custom-flying-emojis March 11, 2022 12:08 View deployment

vercel Bot deployed to Preview – custom-text-chat March 11, 2022 12:08 View deployment

vercel Bot deployed to Preview – fitness March 11, 2022 12:08 View deployment

harshithpabbati marked this pull request as ready for review March 11, 2022 12:08

vercel Bot deployed to Preview – prebuilt-basic-embed March 11, 2022 12:09 View deployment

vercel Bot deployed to Preview – custom-live-streaming March 11, 2022 12:09 View deployment

vercel Bot deployed to Preview – examples March 11, 2022 12:09 View deployment

vercel Bot deployed to Preview – custom-pagination March 11, 2022 12:09 View deployment

vercel Bot deployed to Preview – custom-active-speaker March 11, 2022 12:09 View deployment

vercel Bot deployed to Preview – custom-live-transcription March 11, 2022 12:10 View deployment

ghost reviewed Mar 17, 2022

View reviewed changes

jayne-mast reviewed Mar 17, 2022

View reviewed changes

ghost reviewed Mar 18, 2022

View reviewed changes

jessmitch42 reviewed Mar 23, 2022

View reviewed changes

	className={!participant.video ? 'state error' : 'state success'}
	className={participant.video ? 'state success' : 'state error'}

		@@ -27,32 +31,34 @@ export const RECORDING_IDLE = 'idle';

		export const RECORDING_TYPE_CLOUD = 'cloud';
		export const RECORDING_TYPE_CLOUD_BETA = 'cloud-beta';

		}, [updateRatio]);

		useEffect(() => updateRatio(), [updateRatio]);

Conversation

harshithpabbati commented Mar 10, 2022

Uh oh!

vercel Bot commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

custom-basic-call – ./custom/basic-call

custom-flying-emojis – ./custom/flying-emojis

custom-pagination – ./custom/pagination

custom-text-chat – ./custom/text-chat

custom-live-streaming – ./custom/live-streaming

prebuilt-basic-embed – ./prebuilt/basic-embed

examples – ./custom/live-transcription

custom-live-transcription – ./custom/live-transcription

custom-active-speaker – ./custom/active-speaker

fitness – ./custom/fitness-demo

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayne-mast left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessmitch42 Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rememberlenny commented Mar 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vercel Bot commented Mar 10, 2022 •

edited

Loading

jessmitch42 Mar 21, 2022 •

edited

Loading