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: chat bubble api decision #2508

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Conversation

tewarig
Copy link
Contributor

@tewarig tewarig commented Jan 29, 2025

Description

feat: chat bubble api decision #2508

Changes

Additional Information

Component Checklist

  • Update Component Status Page
  • Perform Manual Testing in Other Browsers
  • Add KitchenSink Story
  • Add Interaction Tests (if applicable)
  • Add changeset

Copy link

changeset-bot bot commented Jan 29, 2025

⚠️ No Changeset found

Latest commit: 8a36c0b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Jan 29, 2025

✅ PR title follows Conventional Commits specification.

Copy link

codesandbox-ci bot commented Jan 29, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8a36c0b:

Sandbox Source
razorpay/blade: basic Configuration

@rzpcibot
Copy link
Collaborator

rzpcibot commented Jan 30, 2025

Bundle Size Report

No bundle size changes detected.

Generated by 🚫 dangerJS against 8a36c0b

saurabhdaware
saurabhdaware previously approved these changes Jan 31, 2025
Copy link
Member

@saurabhdaware saurabhdaware left a comment

Choose a reason for hiding this comment

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

The API looks good to me.

Some comments on doc format but lets pick them up after FTX. You can create a github issue to keep track so that we can fix it later.

```

## Open Questions

Copy link
Member

Choose a reason for hiding this comment

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

So normally we keep the questions in open questions and add conclusion and discussion points based on discussion so that we have reasoning in place if we want to find it later

@@ -0,0 +1,85 @@
# ChatMessage Decisions
Copy link
Member

Choose a reason for hiding this comment

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

You can get the format from other API decision

@@ -0,0 +1,85 @@
# ChatMessage Decisions

Copy link
Member

Choose a reason for hiding this comment

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

You can add annotations image from figma cover of this component like we do in other API decisions


[Figma Link](https://www.figma.com/design/jubmQL9Z8V7881ayUD95ps/Blade-DSL?node-id=100413-32686&t=n9A7LztwEkIsly3v-0)

## ChatMessage Component
Copy link
Member

Choose a reason for hiding this comment

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

We usually keep Usage section first to give idea of the API and then add props table and then more examples of usage


// Markdown
<ChatMessage markdown="# this is markdown" />
```
Copy link
Member

Choose a reason for hiding this comment

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

We can later add pros and cons and why we went with that children API


| Prop | Type | Default | Required | Description |
| ---------------------- | ------------------------------------------- | ------- | -------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| messageType | 'last' or 'default' | default | No | If the message is the last message in the chat and if this prop is enabled we will add decorations messageBubble |
Copy link
Member

Choose a reason for hiding this comment

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

The description of this prop is not clear, what does decorations messageBubble mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update it to -
If messageType is lastmessage in the chat we will add different styles in chat message. currently we have different borderRadius if messageType is last or default |

| isLoading | Boolean | false | No | If the message is loading, we will add a loading animation to the chat bubble |
| validationState | 'error' or 'none' | none | No | if validation state is error we will show error decoration and message|
| errorText | String | null | No | If the message is an error message, we will show the error message in the chat bubble |
| onClick | Function | null | No | callback to be called when ever component is clicked. |
Copy link
Member

Choose a reason for hiding this comment

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

Does it need to be interactive/clickable? If we render a Card inside the slot, it would support its own onClick prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in case of an error, we need to support the onClick prop since children accepts JSX. It should support its own onClick, but in that case, we need to stop event bubbling.

| validationState | 'error' or 'none' | none | No | if validation state is error we will show error decoration and message|
| errorText | String | null | No | If the message is an error message, we will show the error message in the chat bubble |
| onClick | Function | null | No | callback to be called when ever component is clicked. |
| footerActions | ReactNode | null | No | if this is passed we will render this at the end of chat bubble |
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a sample usage for this prop as well? Usually we follow compound API for rendering footer/headers.

Copy link
Contributor Author

@tewarig tewarig Jan 31, 2025

Choose a reason for hiding this comment

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

added -

  <ChatMessage
     footerActions={
       <ChipGroup>
         <Chip value="value1">value1</Chip>
         <Chip value="value2">value2</Chip>
       </ChipGroup>
     }
   >
   demo message
   </ChatMessage>

Comment on lines +55 to +60
// with error
<ChatMessage validationState="error" errorText="Error Message">Demo Text</ChatMessage>

// with card
<ChatMessage><Card></Card></ChatMessage>

Copy link
Member

Choose a reason for hiding this comment

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

How about this API?

// normal string text
<ChatMessage>string message</ChatMessage>

// With a complex body like Mardown/Card
<ChatMessage>
  <ChatMessageBody>
     <Markdown>  Demo Text </Markdown>
     <Card></Card>
  </ChatMessageBody>
  <ChatMessageFooter>
   <Button> Get STarted</Button>
  </ChatMessageFooter>

</ChatMessage>

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 think the Compound Component Pattern would make more sense if we had shared state between the child components.

| onClick | Function | null | No | callback to be called when ever component is clicked. |
| footerActions | ReactNode | null | No | if this is passed we will render this at the end of chat bubble |
| children | 'ReactNode' or 'string' | null | yes | The children that will be rendered inside the chat bubble. can be react node or a string |
| leading | ReactNode | null | No | will be displayed only in case if message is other and also, will contain animation by default |
Copy link
Member

Choose a reason for hiding this comment

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

I think we need a separate prop for just controlling typing animation as well, because otherwise if the component re-mounts/re-renders, the text will animate once again.

Screen.Recording.2025-01-31.at.7.29.29.PM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

typing animation will be controlled by user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would just be adding an animation to the leading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to -
It will be displayed only if the message is 'other' and will also contain an spin animation by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants