Skip to content
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

Feature/f 33 chat implementation #15

Merged
merged 57 commits into from
Nov 8, 2024
Merged

Conversation

Esoteriker
Copy link
Collaborator

@Esoteriker Esoteriker commented Nov 6, 2024

Sorry for later PR, it took some time to refactoring the style issue: especially remove inline style and use tailwind css, it goes smoothly when bug in tailwind config fixed.

There are some points:

  • Regarding backend integration part @ahmtslmngcl will also plan to refactoring sth, I'm not if you need do it in this branch or open a new ticket

  • Typing animation is still pending, current implementation could be delete later, if text is render als markdown from backend side, we will change this component and logic

  • Add some css definition into global.css related to waiting AI response animation, I hope it is not big issue.

  • Since there are a lot custom style variable related to Chatbot, so a lot of customisation has also been added to the tailwind config

  • Since not all icons could be found in iconsax-react, so there is still other package be used.

Haidong Xu and others added 30 commits October 29, 2024 20:14
…implemenation-backend-communication

feat: implement communication with back-end for the chatbot
@Esoteriker Esoteriker marked this pull request as draft November 7, 2024 07:56
Copy link
Collaborator

@Tschonti Tschonti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally, all types, interfaces and props should go into the domain folder, while util functions should be in the operations folder

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use a repository instead of this service file here as well. So you should create an interface called ChatRepository in domain/repositories (with just two methods, testHealth and sendMessage) and a ChatRepositoryImpl in infrastructure/repositories. Then register the implementation in container.tsx so all the components can reach the instance without knowing the exact implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, @ahmtslmngcl will continue refactoring on this part 👍

Copy link
Collaborator

@marinovl7 marinovl7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall AWESOME Job! Just had a few comments regarding code style, other than that functionality, structure, finalization everything rocks! Also generally one thing I noticed in the PR, there are no return types for your functions. Please add return types for each function

const function = (): void => {...}

src/app/page.tsx Outdated Show resolved Hide resolved
src/components/Chatbot/Chatbot.tsx Outdated Show resolved Hide resolved
src/components/Chatbot/Chatbot.tsx Show resolved Hide resolved
src/components/Chatbot/Chatbot.tsx Outdated Show resolved Hide resolved
src/components/Chatbot/Chatbot.tsx Outdated Show resolved Hide resolved
src/components/Chatbot/Chatbot.tsx Outdated Show resolved Hide resolved
src/components/Chatbot/Chatbot.tsx Outdated Show resolved Hide resolved
src/components/Chatbot/ChatbotSidebar.tsx Outdated Show resolved Hide resolved
src/components/TypingText/TypingText.tsx Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not sure if APIError should be placed in here. Would be grateful for feedback!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wanted to rename QueryResponse "response" field to "text" so that the call would be "response.text". But interestingly renaming it (and all its occurences) caused the system to fail when trying to interact with the chatbot.

Copy link

netlify bot commented Nov 8, 2024

Deploy Preview for wfp-hungermap ready!

Name Link
🔨 Latest commit bb64136
🔍 Latest deploy log https://app.netlify.com/sites/wfp-hungermap/deploys/672e6560e4e35f0008a5675f
😎 Deploy Preview https://deploy-preview-15--wfp-hungermap.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Esoteriker Esoteriker requested review from Tschonti and marinovl7 and removed request for Tschonti, marinovl7 and bohdangarchu November 8, 2024 18:43
@Esoteriker Esoteriker marked this pull request as ready for review November 8, 2024 18:52
Copy link
Collaborator

@marinovl7 marinovl7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job! Still few remarks and feedback to be applied, but this PR is big, so we merge it and create smaller PRs for the features and finalizations later

@marinovl7 marinovl7 merged commit f287c0a into main Nov 8, 2024
5 checks passed
@marinovl7 marinovl7 deleted the feature/f-33-chat-implementation branch November 8, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants