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

feat(opentrons-ai-client): add ChatDisplay component #14927

Merged
merged 2 commits into from
Apr 17, 2024

Conversation

koji
Copy link
Contributor

@koji koji commented Apr 16, 2024

Overview

Add ChatDisplay component, its test, and its stories.

The display in Storybook isn't the same as the design since the component doesn't have markdown parse function yet.
The function will be implemented in a following PR.

close AUTH-209 partially

Test Plan

Changelog

  • add ChatDisplay component, its test, and its stories.

Review requests

If you have any better idea for the component name, please let me know. (I don't like the current component name...)

Risk assessment

low

@koji koji marked this pull request as ready for review April 16, 2024 21:10
@koji koji requested a review from a team as a code owner April 16, 2024 21:10
render(props)
screen.getByText('OpentronsAI')
screen.getByText('mock text from the backend')
// Note (kk:04/16/2024) activate the following when jsdom's issue is solved
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if marking these comments as TODOs would be better?

gridGap={SPACING.spacing12}
paddingLeft={isUserInput ? SPACING.spacing40 : undefined}
paddingRight={isUserInput ? undefined : SPACING.spacing40}
// max-width="58.125rem"
Copy link
Collaborator

Choose a reason for hiding this comment

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

will you add this back later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still waiting for the answer from Sue


interface ChatDisplayProps {
text: string
isUserInput: boolean
Copy link
Collaborator

@jerader jerader Apr 17, 2024

Choose a reason for hiding this comment

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

this boolean prop seems okay for now. Is there ever a chance we will allow multi users in the chat at once? If so, maybe changing this to a prop called input and then InputType: 'backend' | 'user' could be better. Feel free to change the prop/type name!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we will allow multi users to use the chat at once.
Actually, I still thinking of the implementation of user input part.

Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

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

lgtm, i left a few suggestions!

@koji koji merged commit 3f9cae7 into edge Apr 17, 2024
6 checks passed
@koji koji deleted the feat_add-chat-part-component branch April 17, 2024 13:17
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
* feat(opentrons-ai-client): add ChatDisplay component
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants