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

Studio: Add animation when we receive a message UI #267

Merged
merged 46 commits into from
Jul 3, 2024

Conversation

kozer
Copy link
Contributor

@kozer kozer commented Jun 18, 2024

Fixes https://github.com/Automattic/dotcom-forge/issues/7679

Proposed Changes

This PR adds animations when the user receives a new message.

Also, the following changes have implemented:

  1. Remove the last message if there is an error
  2. Add a "conversation" feeling by delaying the assistant thinking
  3. Move thinking to be a message instead of passing it as a separate component. That ensures that there is no "bump" effect when thinking is added/removed.

Testing Instructions

  1. Open Studio and navigate to AI Assistant tab
  2. Send a message
  3. When you see the message, make sure there is an animation when it appears, like so:
2024-06-26.22-50-26.mp4

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@kozer kozer requested a review from a team June 18, 2024 10:51
@kozer kozer self-assigned this Jun 18, 2024
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

I tested the changes and worked as expected. However, I'm not approving the PR due to the React warning I shared below.

src/components/content-tab-assistant.tsx Outdated Show resolved Hide resolved
Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

Thanks for adding the animation.
I updated the description of https://github.com/Automattic/dotcom-forge/issues/7679 , let's make the animation from bottom to up, for AI and also User messages.
I think for AI the animation could be in the three dots only, and then replace the text inside.

Current implementation:

message-animation.mp4

@kozer
Copy link
Contributor Author

kozer commented Jun 19, 2024

@sejas , I tried implementing it with tailwind, but I couldn't make it.
It seems that motion div is part of the @wordpress/compoents which are installed, so I ended up using that.
The animation is there, however, is not that good as the video you shared ( probably is the background ).

Please, take a look.

NOTE: All the logic we have ( except the CodeBlock part ), is pretty much a ripoff big-sky UI. I'm wondering if it make sense to switch to that.

@kozer kozer requested review from sejas and fluiddot June 20, 2024 10:53
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

I noticed that we are using like a slide-up animation when the message appears. I'm not sure if it combines well with the animation of scrolling down the content. What do you think about using horizontal animations?

  • Left-to-right for AI assistant messages
  • Right-to-left for user messages

UPDATE: As shared here, the goal is to present messages from bottom to top, so they should be animated vertically.

Additionally, I wonder if we could space out the animations of the user message and the ... (i.e. thinking) message. This way it will mimic a conversation. Currently, both messages show up at the same time.

ai-assistant-animation.mp4

@fluiddot
Copy link
Contributor

Following https://github.com/Automattic/dotcom-forge/issues/7679 and the animation example, I think we need to tweak the scroll and animations a bit to imitate the bottom-to-top flow. The scroll should always remain at the bottom.

@matt-west
Copy link
Contributor

I wonder if we could space out the animations of the user message and the ... (i.e. thinking) message. This way it will mimic a conversation. Currently, both messages show up at the same time.

Agree 👍

When the assistant comes back with a response, is it possible to display the message in the same bubble as the thinking animation?

At the moment, the thinking bubble is removed and a new bubble is added below. This leads to a weird transition where the height of the chat is initially reduced as the thinking bubble is removed, but then it jumps up again once the new message is added. It would be smoother if we could replace the thinking dots with the new message.

Screen.Recording.2024-06-24.at.10.04.33.mov

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

I noticed the following items when testing the latest changes:

Scroll is animated when navigating to the assistant tab

From my side, I think this isn't necessary. I'd expect that the chat starts in the last message without any animation. Additionally, it doesn't scroll completely to the bottom.

ai-animate-scroll-upon-navigation.mp4

Messages are animated when switching sites

Not sure if this effect is on purpose but it might be confusing as it feels like it's removing messages.

ai-message-animated-when-switching-site.mp4

Transition between thinking and AI response

As mentioned by Matt here, it would be great to reuse the same container when transitioning between the thinking state and the AI response. Currently, it's removing message while adding the other.

src/components/content-tab-assistant.tsx Outdated Show resolved Hide resolved
src/components/content-tab-assistant.tsx Outdated Show resolved Hide resolved
src/components/content-tab-assistant.tsx Outdated Show resolved Hide resolved
src/components/content-tab-assistant.tsx Outdated Show resolved Hide resolved
@matt-west
Copy link
Contributor

matt-west commented Jun 27, 2024

@kozer Let's reduce the delay before showing the thinking bubble to 900 milliseconds. That should be long enough for the user to perceive the delay (reinforcing the conversational nature of the interaction) without it feeling like the assistant is unresponsive.

@kozer kozer requested a review from fluiddot July 2, 2024 09:21
@kozer
Copy link
Contributor Author

kozer commented Jul 2, 2024

@fluiddot , @matt-west , can you please check this PR again?
I refactor a bit, and I think now it's working as intended.
If, not, I really want some help here
cc: @sejas

Avoids an awkward scroll jump when the assistant responds with a single line message.
@matt-west
Copy link
Contributor

Thanks @kozer. This looks much better.

I pushed a minor change to fix an issue where the scroll bar jumped if the assistant returned a single line message. It was because the height of the loading bubble is less than the height of a single message. The fix sets a min-height on the loading bubble to prevent this.

I'm not sure if it's related to this PR, but I noticed that markdown isn't always rendered correctly when a message first appears. Sometimes switching tabs or sending another message fixes it.

Screen.Recording.2024-07-02.at.10.59.47.mov

@kozer
Copy link
Contributor Author

kozer commented Jul 2, 2024

Thanks @kozer. This looks much better.

I pushed a minor change to fix an issue where the scroll bar jumped if the assistant returned a single line message. It was because the height of the loading bubble is less than the height of a single message. The fix sets a min-height on the loading bubble to prevent this.

I'm not sure if it's related to this PR, but I noticed that markdown isn't always rendered correctly when a message first appears. Sometimes switching tabs or sending another message fixes it.

Screen.Recording.2024-07-02.at.10.59.47.mov

I'll check this out. Thanks!

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

I noticed Markdown syntax in AI responses. The content is only rendered properly after submitting another prompt:

ai-studio-markdown-bug.mp4

src/components/content-tab-assistant.tsx Outdated Show resolved Hide resolved
src/components/content-tab-assistant.tsx Outdated Show resolved Hide resolved
src/components/content-tab-assistant.tsx Show resolved Hide resolved
src/components/chat-message.tsx Outdated Show resolved Hide resolved
src/components/tests/content-tab-assistant.test.tsx Outdated Show resolved Hide resolved
src/components/tests/content-tab-assistant.test.tsx Outdated Show resolved Hide resolved
src/components/tests/content-tab-assistant.test.tsx Outdated Show resolved Hide resolved
src/components/tests/content-tab-assistant.test.tsx Outdated Show resolved Hide resolved
src/components/tests/content-tab-assistant.test.tsx Outdated Show resolved Hide resolved
src/components/tests/content-tab-assistant.test.tsx Outdated Show resolved Hide resolved
@kozer
Copy link
Contributor Author

kozer commented Jul 2, 2024

@matt-west , it should have been fixed now.
@fluiddot , made changes as requested.

@kozer kozer requested a review from fluiddot July 2, 2024 11:42
@matt-west
Copy link
Contributor

Yep, looks good now. Thanks @kozer!

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

LGTM 🎊 ! Thanks @kozer for addressing the comments I shared 🙇 !

src/components/chat-message.tsx Show resolved Hide resolved
@kozer kozer merged commit 2188ae0 into trunk Jul 3, 2024
10 checks passed
@kozer kozer deleted the add/animation_chat_message branch July 3, 2024 06:28
aria-labelledby={ id }
className={ cx(
'inline-block p-3 rounded border border-gray-300 overflow-x-auto select-text',
'inline-block p-3 rounded border overflow-x-auto select-text',
Copy link
Contributor

Choose a reason for hiding this comment

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

@kozer Is there a reason we are adding this line twice - once with border-gray-300 and the second tine without it but with all other identical classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch Kat! No, just leftover from while no was working on it. Feel free to clean it up! Thanks for looking at this one!

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.

5 participants