-
Notifications
You must be signed in to change notification settings - Fork 403
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
refactor: observer in MessageSeen.jsx #676
Conversation
@damirgros is attempting to deploy a commit to the dunsin's projects Team on Vercel. A member of the Team first needs to authorize it. |
@damirgros great work just one thing, can you replace the code with a text description of what you did this is because we can see the code in the code tab already, but we won't see a text description. And a good convention is to delete a fork to follow a linear history on GitHub, this is so your code doesn't combine the last code which was already merged. Or you could just create a new branch from the main and then pull it so it's linear (for this one, no need to delete the PR just a correction) |
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
Intersection Observer API doesn't have connect method. Can you try in production only changing useEffect in MessageSeen.jsx to look like this (I added the check for if observer and sortedMessages exist, removed observer.connect and added observer dependancy): useEffect(() => {
// Only proceed if the observer and sortedMessages are available
if (!observer || !sortedMessages.length) return;
sortedMessages.forEach((message) => {
if (message.isRead) return;
const messageElement = document.getElementById(`message-${message.id}`);
if (messageElement && isTabVisible) {
observer.observe(messageElement);
}
});
return () => {
if (observer) observer.disconnect();
};
}, [sortedMessages, isTabVisible, observer]); |
Push it to GitHub and I'll set it up here |
It is pushed. |
@damirgros Alright it works in prod, also address this please the first one |
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.
looks great, thanks for it.
Added text description in the PR |
Fixes Issue
My PR closes #669
π¨βπ» Changes proposed(What did you do ?)
Refactored observer logic in the "MessageSeen.jsx" by creating useObserver hook in the new file "useObserver.js" in the folder "hooks".
Transfered observer logic from "MessageSeen.jsx" to "useObserver.js" and also memoized it so it doesn't get re-created every time component re-renders.
Removed unused imports from "MessageSeen.jsx" and initiated observer variable with useObserver hook.
βοΈ Check List (Check all the applicable boxes)
Note to reviewers
π· Screenshots