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

Add horizontal resize of ai chat panel #983

Merged
merged 11 commits into from
Jan 23, 2024

Conversation

FadhlanR
Copy link
Contributor

Ticket: CS-6430

Screen.Recording.2024-01-18.at.14.35.26.mov

Copy link

github-actions bot commented Jan 18, 2024

Test Results

490 tests  ±0   486 ✔️ ±0   6m 39s ⏱️ +14s
    1 suites ±0       4 💤 ±0 
    1 files   ±0       0 ±0 

Results for commit bb31c6c. ± Comparison against base commit 4e2e934.

♻️ This comment has been updated with latest results.

@backspace
Copy link
Contributor

I’m not sure of the boundaries of this and other issues, maybe this is 6485, but I noticed I’m unable to scroll the messages container and when I tab to the field to add another message everything shifts upward:

screencast 2024-01-18 12-34-16

The proximity detection for showing and highlighting the handle is great, I found the snapping at the minimal width a bit jarring, unclear how that could be improved though

screencast 2024-01-18 12-41-40

@backspace
Copy link
Contributor

I also meant to ask: I’d expect the open/closed state of the panel to be persistent across refreshes, as well as its width. I don’t see an open issue for this and realise it’s not in the issue as defined though!

@FadhlanR
Copy link
Contributor Author

FadhlanR commented Jan 19, 2024

@backspace , Thank you for the feedback. I've addressed the scroll issue in the AI Assistant panel and added a collapsible argument to ResizablePanel. By default, this argument is set to true, but if it's set to false, the ResizablePanel won't be collapsed.

Screen.Recording.2024-01-19.at.11.51.26.mov

@FadhlanR
Copy link
Contributor Author

I also meant to ask: I’d expect the open/closed state of the panel to be persistent across refreshes, as well as its width. I don’t see an open issue for this and realise it’s not in the issue as defined though!

@backspace do you think it is better to make the open/closed and width state persistent in this PR?

@backspace
Copy link
Contributor

backspace commented Jan 19, 2024

do you think it is better to make the open/closed and width state persistent in this PR?

I’m uncertain! Let’s just say leave it for now, something to consider for the future.

The scrolling and non-collapse look good, thanks!

ETA something seems to be terribly wrong with the tests and therefore Percy, let us know when it’s ready again

@FadhlanR FadhlanR force-pushed the cs-6430-horizontal-resize-of-ai-chat-panel branch from 8b67bcc to 1151ac5 Compare January 22, 2024 14:52
@FadhlanR
Copy link
Contributor Author

I've fixed boxel ui test and host test. This PR is ready for another review. cc: @backspace

Copy link
Contributor

@backspace backspace left a comment

Choose a reason for hiding this comment

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

looking good, I appreciate all the refinements 🎉

@jurgenwerk
Copy link
Contributor

I think there should be a limit how much you can drag this to the left. This happens if you drag it all the way to the left:

image

@lukemelia
Copy link
Contributor

Is the ResizeHandler component the thing responsible for rendering the handle? If so, should it be called ResizeHandle instead of ResizeHandler? I usually think of a "handler" as something that responds to events being triggered.

@FadhlanR
Copy link
Contributor Author

@jurgenwerk I've added minimum length and made collapsibale to false for stack items panel. So we cannot make the stack items panel disappear.

Screen.Recording.2024-01-23.at.14.32.47.mov

@FadhlanR FadhlanR merged commit b0d1481 into main Jan 23, 2024
22 checks passed
@delete-merged-branch delete-merged-branch bot deleted the cs-6430-horizontal-resize-of-ai-chat-panel branch January 23, 2024 12:17
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