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

cody: cleanup the design of the VSCode History view #51246

Merged
merged 8 commits into from
May 23, 2023

Conversation

toolmantim
Copy link
Contributor

As a warmup to starting work on the VSCode Extension, I did some cleanups of the design of the Cody VSCode chat history. This pushes the most important content in the history panel (the history items themselves) much higher, and moves the clear history to just a button next to the header (instead of a whole section).

Before After
Screenshot 2023-04-28 at 5 17 40 pm Screenshot 2023-04-28 at 5 18 27 pm

Notes:

  • In the process of battling the default button styles, I switched it to use standard VSCodeButton elements, which give us a bunch of nice defaults for free (hover etc).
  • There's probably more work that can be done in follow up PRs to bring it up to speed with Add cody web history ui #51072, and any new work in [Design]: Cody chat history view (new design) #51165, but this at least improves what we have now.
  • There was a keyboard handler and tabindex in there, but I couldn't seem to get it to trigger. Would love help for adding it back in if it was working.

Future Fixes / Discussions

  1. When starting your first chat, and then clicking history, I was expecting it to appear in Chat History. But it's more like "Previous Chat Sessions" — you have to reload, or leave and come back, to have new things appear. I feel like the current live session could still be there at the top of the History (if there's been a message sent), and clicking it would just go back to it.
  2. When clicking on a recipe, I thought it would start a new chat, and the old chat would end up in history. But it just adds to the current chat 🤔
  3. If you use fixup, the chat messages (and therefore chat history buttons) are a bit empty and awkward. Screenshot 2023-04-28 at 5 49 23 pm Screenshot 2023-04-28 at 5 51 04 pm
    Perhaps it could say "I've updated your code." instead of "Here is my attempt at rewriting the selected code:"
  4. Sometimes the history buttons contain escaped HTML, which would normally be rendered as HTML in the normal chat. It might be nice to fix that.

Test plan

Would appreciate another person’s manual test of this, given it's my first contribution to the extension 🙏🏼

  • With no history, open Chat History view, verify it works okay, and that Clear History button is disabled
  • Perform a chat, reload the window, open Chat History view, verify buttons and labels
  • Perform another chat, reload the window, open Chat History view, verify buttons and labels
  • Click Clear History

@toolmantim toolmantim requested review from SuperAuguste, abeatrix and a team April 28, 2023 08:03
@cla-bot cla-bot bot added the cla-signed label Apr 28, 2023
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

nice 🚀

Copy link
Contributor

@SuperAuguste SuperAuguste left a comment

Choose a reason for hiding this comment

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

I love it! Awesome design Tim!! (also tysm to much for cleaning up that CSS and putting it in a module 🙏)

Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

Apart from the overflow issues as mentioned, I really like the new design, def much better than what we have now!

}

return (
<VSCodeButton
Copy link
Contributor

@abeatrix abeatrix Apr 28, 2023

Choose a reason for hiding this comment

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

We replaced VSCodeButton with div originally because of an UI glitch that caused a weird line to show up when the button is wider than 300px.

Edit: fixed a typo. Want to add that the recipe page also have the same problem so this is not a request for change on this issue, just wanted to provide some background info 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a screenshot that shows the weird line in each buttons as reported previously in #50960 :
image

margin: 0 1rem;
display: flex;
gap: 1rem;
flex-direction: column;
Copy link
Contributor

Choose a reason for hiding this comment

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

We might need to add overflow and wrap to the item container:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh good catch! Thanks @abeatrix. I've just updated it back to using our own <button> (styling the shadow root element is impossible), and fixed recipe buttons while I was there.

I couldn't replicate the wrapping problem though. Could you check this PR again with your local data? Thank you!

@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented May 4, 2023

📖 Storybook live preview

@abeatrix
Copy link
Contributor

abeatrix commented May 4, 2023

I'm still seeing the overflow issue, maybe it's just me? 🤔
The texts are pushed to the side cause of a p
image

@abeatrix
Copy link
Contributor

abeatrix commented May 4, 2023

Looks like it's fixed, we will need to update the dependencies for this: microsoft/vscode-webview-ui-toolkit#384

abeatrix added a commit that referenced this pull request May 10, 2023
…1726)

Close #51719
Unblock #51246

Thanks @eseliger and @philipp-spiess for the help with [upgrading the
packages](https://sourcegraph.slack.com/archives/C052G9Y5Y8H/p1683728783413159)
🙇

## Issues

There was an issue with VS Code Webview buttons displaying a light
border due to max-width set to 300 in the upstream package, which was
then fixed in
microsoft/vscode-webview-ui-toolkit#384


![image](https://github.com/sourcegraph/sourcegraph/assets/68532117/3143b960-3008-4f31-b409-39aafa7ad8ab)

Text is squeezed to the side due to max width issue:

![image](https://github.com/sourcegraph/sourcegraph/assets/68532117/7e77d525-6f00-4f77-8d5a-18bf06142edb)

## Fixs

After updating the package, there is still a border for the height, but
the width constrain has been removed which solve the issue where the
text is squeezed to the side:
<img width="557" alt="image"
src="https://github.com/sourcegraph/sourcegraph/assets/68532117/b07cbd16-2ae5-46e7-bca9-8d1459fc076c">

Overriding the border color manually worked:

<img width="616" alt="image"
src="https://github.com/sourcegraph/sourcegraph/assets/68532117/72afcb1d-cf4a-4785-83fc-6d99438ddb71">


## Test plan

<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->

Package updated and working fine locally. See screenshots above
all: unset;
box-sizing: border-box;
display: inline-flex;
justify-content: center;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
justify-content: center;
justify-content: flex-start;
width: 100%;

To fix centered text issue:

image

Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

I've updated the package that has the fix for the max-width issue and looks like it works with your changes too.
The only issue is the justify-content: center; I mentioned above but other than that i think it's good to go :D

Here is what it looks like on my side after updating from center to flex-start with width set to 100%:
image

image

@toolmantim
Copy link
Contributor Author

Amazing, thank you @abeatrix! I'll get this cleaned up and ready to go.

@toolmantim toolmantim removed the request for review from danielmarquespt May 23, 2023 08:56
@toolmantim
Copy link
Contributor Author

This is ready to go

@toolmantim toolmantim merged commit b4fa9c9 into main May 23, 2023
24 checks passed
@toolmantim toolmantim deleted the cody-vscody-chat-history-design-cleanups branch May 23, 2023 09:46
VolhaBakanouskaya pushed a commit to VolhaBakanouskaya/sourcegraph-public that referenced this pull request Jun 30, 2023
…1726)

Close sourcegraph/sourcegraph#51719
Unblock sourcegraph/sourcegraph#51246

Thanks @eseliger and @philipp-spiess for the help with [upgrading the
packages](https://sourcegraph.slack.com/archives/C052G9Y5Y8H/p1683728783413159)
🙇

## Issues

There was an issue with VS Code Webview buttons displaying a light
border due to max-width set to 300 in the upstream package, which was
then fixed in
microsoft/vscode-webview-ui-toolkit#384


![image](https://github.com/sourcegraph/sourcegraph/assets/68532117/3143b960-3008-4f31-b409-39aafa7ad8ab)

Text is squeezed to the side due to max width issue:

![image](https://github.com/sourcegraph/sourcegraph/assets/68532117/7e77d525-6f00-4f77-8d5a-18bf06142edb)

## Fixs

After updating the package, there is still a border for the height, but
the width constrain has been removed which solve the issue where the
text is squeezed to the side:
<img width="557" alt="image"
src="https://github.com/sourcegraph/sourcegraph/assets/68532117/b07cbd16-2ae5-46e7-bca9-8d1459fc076c">

Overriding the border color manually worked:

<img width="616" alt="image"
src="https://github.com/sourcegraph/sourcegraph/assets/68532117/72afcb1d-cf4a-4785-83fc-6d99438ddb71">


## Test plan

<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->

Package updated and working fine locally. See screenshots above
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

5 participants