-
Notifications
You must be signed in to change notification settings - Fork 2
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
Demo: Refactor demo app to reduce single component complexity. #433
base: main
Are you sure you want to change the base?
Conversation
Coverage Report
File CoverageNo changed files found. |
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 (
|
cf9d0d6
to
7f9e668
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.
looks much better! i think most of these comments are just relating to stuff you copied out of Chat.tsx
so feel free to ignore if that's the case
const { send } = useMessages(); | ||
const { start, stop } = useTyping(); | ||
const { currentStatus } = useChatConnection(); | ||
const shouldDisable: boolean = currentStatus !== ConnectionStatus.Connected; |
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're doing this two different ways in this component and in ReactionComponent
; here we calculate whether we're not connected each render and in ReactionComponent
we have a useEffect
and store the result in component state (I think this approach is slightly better). Should we unify the approach, or otherwise, if we anticipate users needing to do this often, should useChatConnection
just return a connected
boolean as part of the returned object?
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.
Yeh we've discussed this before, though there are a few ways we could solve this so for now I will standardise the check and add a ticket to improve this.
className="chat-window" | ||
> | ||
{messages.map((msg) => { | ||
console.log(msg); |
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.
don't think you meant to commit this line 😄
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.
Good spot, left in after some debugging :D
7f9e668
to
2eb9e7b
Compare
2eb9e7b
to
5de8b6e
Compare
I've had a look at this. I noticed the chat window was no longer full height. I've had a go at adjusting some CSS #447 to make it full height again Will finish reviewing tomorrow, I'm keen to get this merged before doing any more work on the demo app in the message reactions branch |
5de8b6e
to
aa737b9
Compare
3248514
to
e4c9304
Compare
Refactored the demo app Chat container component. Everything was packed into this one container, so it made it confusing to understand the separation of features. This is a first pass, for now I have just moved things into their own components, but we should look at ensuring each is fully isolated and reusable.
e4c9304
to
b460e38
Compare
Everything was packed into this one container, so it made it confusing to understand the separation of features. This is a first pass, for now I have just moved things into their own components, but we should look at ensuring each is fully isolated and reusable.
Testing Instructions (Optional)