-
Notifications
You must be signed in to change notification settings - Fork 351
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
Chat: context cell improvements #6115
Conversation
2c8d18f
to
e8aed40
Compare
…re not resolvable
83f90c3
to
d09e55e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this in Cody Web in Sourcegraph and it works as expected, left one questions about wierd display:none CSS rule
@@ -10,7 +10,8 @@ | |||
} | |||
|
|||
.context-item-edit-button { | |||
display: flex; | |||
/* display: flex; */ | |||
display: none; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is wrong; it works because I think the edit button is a Button, and it uses tailwind d-flex class, which overrides display:none
here, but just double check what you want to achieve with display none here. Do you want to hide this edit button?
Or it's just a leftover
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this PR description description says
"This PR also removes the "Edit context" button "
But I still see edit context / edit results button in JSX, should we just remove it completely from rendering rather than just hide it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to add back the "Edit Context" button soon, and this was the quickest way to do it. I'll submit a follow-up PR to either remove it completely or fix the functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good.
But I think design may want to have a better UX here: cc @taiyab for design review.
@taiyab happy to consider proposals to improve UX, but this is better than what we have in prod today, so I will merge. |
@thenamankumar @beyang Yea, all good. This is objectively better than existing. We'll bring another iteration to this soon if need be. |
On a context-less chat request, we show a "none fetched" indicator in the ContextCell, along with a "Resend with current repository context" button:
![Selection_910](https://private-user-images.githubusercontent.com/1646931/385995926-5faefc2e-4bb5-488f-bbba-52aee2f300ea.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkxMzYzMzYsIm5iZiI6MTczOTEzNjAzNiwicGF0aCI6Ii8xNjQ2OTMxLzM4NTk5NTkyNi01ZmFlZmMyZS00YmI1LTQ4OGYtYmJiYS01MmFlZTJmMzAwZWEucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDlUMjEyMDM2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9YzNjMjc1ZGFiM2Y1YzBjMGE0ZGU5NDBhMzFkZjYzM2YxNmJlYTllMzhlNDk2ZDkyYjRkNmI1NzAxNTQyZTk2MSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.4qHGbxFFRHBSKhFMY_kOBKN7o1ZNKt5sLJ0JGPGjLwg)
Clicking "Resend with current repository context" causes the repository chip to be added and the request resent:
![Selection_911](https://private-user-images.githubusercontent.com/1646931/385995953-8f665570-1266-46d7-84a9-43700992480e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkxMzYzMzYsIm5iZiI6MTczOTEzNjAzNiwicGF0aCI6Ii8xNjQ2OTMxLzM4NTk5NTk1My04ZjY2NTU3MC0xMjY2LTQ2ZDctODRhOS00MzcwMDk5MjQ4MGUucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDlUMjEyMDM2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9MzRhYWRkYTdkODg3OTA3ZjJkNDU4NjhmOGI1MDI3ZjA3NDc4NTdjNTZkM2UzMzQ5NzgzYmFiOWJiMGFhNjViMyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.vEInXFyP461RTVgYahkLuxLDORtCgqCnmzl0fehsTUE)
The "Resend with current repository context" also appears when context is at-mentioned but there is no repo chip:
![Selection_912](https://private-user-images.githubusercontent.com/1646931/385995969-9041d5c2-241c-4f63-94e4-da93ed8116cc.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkxMzYzMzYsIm5iZiI6MTczOTEzNjAzNiwicGF0aCI6Ii8xNjQ2OTMxLzM4NTk5NTk2OS05MDQxZDVjMi0yNDFjLTRmNjMtOTRlNC1kYTkzZWQ4MTE2Y2MucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDlUMjEyMDM2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9YWE4YzI2NTM3NWJkYTM1NzJmYWZmM2Q1ODQ1MjcwOGM1ODhmZjYxNThkZjE0MjA4NWY1MGI2ZTY1MjRhZWViMCZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.Ip5Z39Epzt8X93DV19NkG6Cn82GMahcHoeL9hu25goU)
This PR also removes the "Edit context" button (which previously caused the fetched context items to be moved to the input), because this was broken and not actually fetching context.
Test plan
Test in local dev the scenarios in the screenshots above.