-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[EDU-1664] - Chat Online status example #2302
base: main
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
825ca88
to
6fe6a8d
Compare
28d7fe0
to
984c06e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should make the 'away' indicator yellow rather than grey, as grey is normally for offline. Thoughts?
747e29c
to
472abaa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't notice it before, but we should have a (You)
label against your own entry.
Can we make the button fixed width too, so that it doesn't resize when you change your status?
7557ab1
to
470606c
Compare
e6d2e7b
to
65de162
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You just need to add an entry for the usePresence
hook in page.md
but everything else LGTM
d8479e6
to
66f68fe
Compare
73fde35
to
7f6aec9
Compare
ce90cf7
to
fb70c3c
Compare
7f6aec9
to
8c4d5ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small things, otherwise it looks good!
// Get ROOM with typing capabilities | ||
const room = chatClient.rooms.get('chat-online-status', { presence: RoomOptionsDefaults.presence }); | ||
|
||
const onlineStatuses = await room.presence.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't normally have awaits at the top level of a file, is this common practice in examples for simplicity? If there is a need for this, perhaps update the module to ECMAScript 2022?
const card = document.createElement('div'); | ||
card.className = | ||
'flex items-center p-4 bg-gray-100 rounded-lg shadow-sm hover:shadow-md transition-shadow duration-200'; | ||
card.id = onlineStatus.clientId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a possibility of name collision here. Probably small, so maybe it's not an issue to worry about, but I'd double-check the randomness of fakers firstName() function, just ensure it's a big enough dataset that the chance is low
|
||
const onlineStatuses = await room.presence.get(); | ||
|
||
async function addCard(onlineStatus: PresenceMember | PresenceEvent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth some small comments here to separate the code maybe? e.g..
// Create an element to store status items
const card = document.createElement('div');
card.className =
'flex items-center p-4 bg-gray-100 rounded-lg shadow-sm hover:shadow-md transition-shadow duration-200';
card.id = onlineStatus.clientId;
// Create an element to store status data..
} | ||
|
||
onlineStatuses.forEach((onlineStatus: PresenceMember) => { | ||
addCard(onlineStatus); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary that these populate before you subscribe to presence? It looks like addCard is async, but I can't see anything there that requires awaiting?
/** 💡 Subscribe to the presence set of the room to see online statuses 💡 */ | ||
room.presence.subscribe((event: PresenceEvent) => { | ||
if (event.action === 'enter') { | ||
addCard(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, it looks like we could just make this sync?
const Online = () => { | ||
const [onlineStatuses, setOnlineStatuses] = useState<PresenceEvent[]>([]); | ||
const { presenceData, error } = usePresenceListener({ | ||
listener: (presence) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listener: (event: PresenceEvent)
|
||
const Online = () => { | ||
const [onlineStatuses, setOnlineStatuses] = useState<PresenceEvent[]>([]); | ||
const { presenceData, error } = usePresenceListener({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems error isn't used?
const { presenceData, error } = usePresenceListener({ | ||
listener: (presence) => { | ||
if (presence.action === 'enter') { | ||
setOnlineStatuses((prevStatus: PresenceEvent[]) => [...prevStatus, presence as PresenceEvent]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can now remove
as PresenceEvent
if (presence.action === 'enter') { | ||
setOnlineStatuses((prevStatus: PresenceEvent[]) => [...prevStatus, presence as PresenceEvent]); | ||
} else if (presence.action === 'leave') { | ||
setOnlineStatuses((prevStatus: PresenceEvent[]) => prevStatus.filter((status: PresenceEvent) => status.clientId !== presence.clientId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does presence.action === 'update'
need to be handled here? Also, if so, could we turn this into a switch? :)
const { clientId } = useChatClient(); | ||
|
||
useEffect(() => { | ||
if (presenceData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea to show both methods to retrieve presence data? I'm not sure what the docs look like, but this could be confusing? Perhaps just use one and console log the other? Also maybe some comments to let the user know that both methods are valid and shown for demonstration?
fb70c3c
to
ef141a1
Compare
8c4d5ef
to
9f1e202
Compare
This PR contains the two code samples for the new Examples section we are creating. It contains a Typescript and a React version for Online status with Chat. Which will be rendered in the code sandbox live examples in the future, along with others.
The README within each directory /examples/chat-online-status/javascript and /examples/chat-online-status/react contains information on how to get these running.
Jira epic ticket
Jira ticket.