Skip to content

Remove the use of @vscode/webview-ui-toolkit #3986

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

Merged
merged 45 commits into from
Apr 16, 2025

Conversation

tuan-nguen
Copy link
Contributor

@tuan-nguen tuan-nguen commented Apr 3, 2025

This removes the use of @vscode/webview-ui-toolkit and replaces it with @vscode-elements/react-elements. The PR could not broken down to smaller PRs because importing both libraries breaks the view tests. Both libraries register similarly named components (e.g. vscode-button) which have conflicting implementations and that causes an error when tests are run.

I have created a new component ActionButton which aims to copy the styles that @vscode/webview-ui-toolkit for VSCodeButton when appearance="icon". I did this because the new library renders the background even when it's an icon button.

I have also added a new component Badge which applies variant="counter" by default which applies a border-radius to the badge.

I have also replaced getBy* queries with findBy* in view tests because getBy* doesn't seem to load elements with all their attributes; however, findBy* selects elements only when all the attributes have been loaded.

@tuan-nguen tuan-nguen marked this pull request as ready for review April 8, 2025 08:13
Copy link
Contributor

@robertbrignull robertbrignull 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 making these changes and making the new components where necessary. From a code perspective everything looks fine. I haven't had time to look at the tests yet.

For the visual changes, I've taken some before/after screenshots we can hopefully use for comparison. There are quite a few changes, and some are fine as I guess these new components just have a slightly different style, but some I think we might want to address to stick to the intended designs.

Variant analysis page

Before:
Screenshot 2025-04-08 at 16 23 15

After:
Screenshot 2025-04-08 at 16 35 06

Changes I can see are:

  • Clickable text isn't blue anymore. Maybe this is an acceptable change but my inclination is we probably want to keep it blue.
  • Icons in the dropdowns + search bar have gone. Is it possible to keep these?
  • Padding for the "copy repository list" + "export results" buttons has changed a minuscule amount but it's still fine.
  • Padding for the dropdowns + search bar has decreased quite a bit. It's still readable but maybe not as aesthetically pleasing as it was before.
  • The ">" at the start of each results row has more padding. It's not bad but it's also not necessary so might be worth reclaiming that horizontal space if we can.

Model editor

Before:
Screenshot 2025-04-08 at 16 25 39

After:
Screenshot 2025-04-08 at 16 36 52

Changes I can see are:

  • Clickable text isn't blue anymore. Same as the variant analysis page.
  • Button size / padding has changed a tiny bit, but it's fine. Same as the variant analysis page.
  • The horizontal divider is perhaps a bit chunky now.
  • The numbers in the badges (e.g. just before the "view" button) aren't as centered as they used to be. The centering wasn't actually perfect before either, but I think it's become more off.

@tuan-nguen
Copy link
Contributor Author

Icons in the dropdowns + search bar have gone. Is it possible to keep these?

Padding for the dropdowns + search bar has decreased quite a bit. It's still readable but maybe not as aesthetically pleasing as it was before.

I have a fixed the icons and the padding for the search bar but the dropdown, which is based on VscodeSingleSelect, doesn't allow having icons in the input area and the padding is fixed. I have looked through the documentation and the source code and couldn't find a way to pass them as props.

One possible solution is to create our own Dropdown which copies the styles for the VSCodeDropdown component from @vscode/webview-ui-toolkit. However, the styles contain a lot of different cases such as when it's disabled, opened and hidden. What do you think? @robertbrignull

Copy link
Member

@koesie10 koesie10 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 working on this! I haven't taken a very close look yet, I've just read through most of the code and have some initial comments based on that.

@robertbrignull
Copy link
Contributor

Something's going a little wrong in the variant analysis view and I'm getting 0s being displayed

Screenshot 2025-04-14 at 17 06 32

I've seen similar things before where something is a falsy value and you hope it will just not be shown at all but it does end up being displayed (in this case as a 0). I had a quick 2 minute look in the code and couldn't spot where it was coming from, but as discussed on slack I'm happy to help debug on Wednesday if that's helpful.

@robertbrignull
Copy link
Contributor

Other than that comment above all the styling looks great. Thanks for updating that!

Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

LGTM and we just paired on the final changes.

Ready to go from my perspective, unless @koesie10 would like to have another look.

Copy link
Member

@koesie10 koesie10 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 working on this! It's looking really good and I couldn't find any styling issues in all of the views I checked.

@tuan-nguen tuan-nguen requested a review from koesie10 April 16, 2025 14:15
Copy link
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

Thanks!

@tuan-nguen tuan-nguen merged commit 8318e04 into main Apr 16, 2025
18 of 19 checks passed
@tuan-nguen tuan-nguen deleted the tuan-nguen/replace-vscode-webview branch April 16, 2025 14:56
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.

3 participants