-
Notifications
You must be signed in to change notification settings - Fork 75
[LG-5932, LG-5934] refactor,feature(chat) Compound MessageFeed #3488
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
base: s/initial-message-integration
Are you sure you want to change the base?
[LG-5932, LG-5934] refactor,feature(chat) Compound MessageFeed #3488
Conversation
…afygreen-ui into LG-5932-message-feed-compound
🦋 Changeset detectedLatest commit: bd83d5f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Pull request overview
This PR refactors MessageFeed into a compound component and introduces a new context provider. The changes prepare the component for more flexible composition while temporarily removing the assistant name display from messages.
Changes:
- Refactored
MessageFeedto use theCompoundComponentpattern - Added
MessageFeedContextwith provider and custom hook for managing feed state - Removed assistant avatar and name display from non-sender messages in
Messagecomponent
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| chat/message/src/Message/Message.tsx | Removes assistant avatar/name rendering and related dependencies |
| chat/message-feed/src/MessageFeedContext/index.ts | Exports new context provider and hook |
| chat/message-feed/src/MessageFeedContext/MessageFeedContext.types.ts | Defines types for the MessageFeed context |
| chat/message-feed/src/MessageFeedContext/MessageFeedContext.tsx | Implements context provider and hook with error boundary |
| chat/message-feed/src/MessageFeedContext/MessageFeedContext.spec.tsx | Adds test coverage for the context hook |
| chat/message-feed/src/MessageFeed/MessageFeed.tsx | Wraps component with CompoundComponent wrapper |
| chat/message-feed/shared.types.ts | Defines subcomponent property constants for compound pattern |
| .changeset/dark-pots-tell.md | Documents changes for release notes |
chat/message-feed/src/MessageFeedContext/MessageFeedContext.tsx
Outdated
Show resolved
Hide resolved
|
Size Change: +63 B (0%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
|
Coverage after merging LG-5932-message-feed-compound into s/initial-message-integration will be
Coverage Report for Changed Files
|
|||||||||||||||||||
.changeset/dark-pots-tell.md
Outdated
| @@ -0,0 +1,6 @@ | |||
| --- | |||
| '@lg-chat/message-feed': path | |||
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.
| '@lg-chat/message-feed': path | |
| '@lg-chat/message-feed': patch |
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.
actually, this should be a minor since we're adding this context feature
| } | ||
| ] | ||
| } | ||
| } |
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.
lint: add empty line
| }, | ||
| ), | ||
| { | ||
| displayName: 'MessageFeed', |
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.
do we need to add a key here? I dont have too much context on the larger project but im not sure where MessageFeedSubcomponentProperty.InitialMessage should be going
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.
Yes! I will add it in the next PR.
✍️ Proposed changes
Changes in the PR include:
MessageFeedinto a compound componentMessageFeed🎟 Jira ticket: LG-5934
🎟 Jira ticket: LG-5934
✅ Checklist
For new components
For bug fixes, new features & breaking changes
pnpm changesetand documented my changes🧪 How to test changes