-
Notifications
You must be signed in to change notification settings - Fork 200
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
base: main
Are you sure you want to change the base?
Conversation
…uan-nguen/replace-vscode-webview
extensions/ql-vscode/src/view/method-modeling/MultipleModeledMethodsPanel.tsx
Fixed
Show fixed
Hide fixed
extensions/ql-vscode/src/view/method-modeling/MultipleModeledMethodsPanel.tsx
Fixed
Show fixed
Hide fixed
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.
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
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
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.
extensions/ql-vscode/src/view/common/ActionButton/ActionButton.tsx
Outdated
Show resolved
Hide resolved
extensions/ql-vscode/src/view/variant-analysis/RepositoriesResultFormat.tsx
Show resolved
Hide resolved
I have a fixed the icons and the padding for the search bar but the dropdown, which is based on One possible solution is to create our own |
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.
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.
@@ -18,6 +18,7 @@ | |||
--vscode-descriptionForeground: rgba(204, 204, 204, 0.7); | |||
--vscode-icon-foreground: #c5c5c5; | |||
--vscode-focusBorder: #007fd4; | |||
--vscode-contrastActiveBorder: #f38518; |
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.
Did you use the instructions in the Storybook/at the top of this file to update these variables? If so, I'm surprised that there's only 1 new variable.
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 added this manually into this file because the ActionButton
uses this variable to show a border but after updating the variables using the method you pointed out still doesn't include this variable for the dark and light theme. Do we then allow the button not to have a border in those themes?
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.
If it's not in the styles that you can copy from within VS Code, then it means that this variable is not available to use inside VS Code. These stylesheets are only used for Storybook and are not actually used within VS Code, the variables on that html
element are used instead. Is there another variable that would work for this that is available for use in web views?
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.
There doesn't seem to be another variable that can be used. I can add a default value in the ActionButton.module.css
file if the variable can't be found but the value should be different in dark and light themes.
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.
Is the value currently different in dark and light modes? If so, how does the component currently get the correct color value to use?
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 checked in storybook and the value seems to be the same, I'm not sure why I remember them being different. I will just add the default value for it to copy the style from here.
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 Storybook is not fully accurate since that just uses the same CSS stylesheet which is only copied from VS Code. Is the value also the same in VS Code?
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.
In VS Code it seems to default to transparent
in both default dark and default light theme
extensions/ql-vscode/src/view/common/ActionButton/ActionButton.css
Outdated
Show resolved
Hide resolved
…uan-nguen/replace-vscode-webview
Other than that comment above all the styling looks great. Thanks for updating that! |
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
forVSCodeButton
whenappearance="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 appliesvariant="counter"
by default which applies a border-radius to the badge.I have also replaced
getBy*
queries withfindBy*
in view tests becausegetBy*
doesn't seem to load elements with all their attributes; however,findBy*
selects elements only when all the attributes have been loaded.