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

Removing & replacing react elements from deprecated webview-ui-toolkit package. #1239

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tejhan
Copy link
Collaborator

@tejhan tejhan commented Feb 12, 2025

This incremental PR serves to replace a large amount of react elements from the deprecated webview-ui-toolkit package.

Most prominently, almost all instances of VSCodeButton, VSCodeLink, VSCodeDivider, & VSCodeCheckbox have been completely removed & replaced with properly styled alternatives.

Other elements like VSCodeTextField, VSCodeDropdown haven't been fully replaced yet, but styles have been created & applied in some scenarios. (Our current implementation requires case by case changes for each instance, so we have to make sure to test for each case before blanket replacement.) Full replacement will come in follow up PR.

For future reference:

  • .secondary-button replaces VSCodeButton's appearance="secondary"
  • .icon-button replaces VSCodeButton's appearance="icon"
  • input[type="text'] replaces VSCodeTextInput

.vsix for testing:
vscode-aks-tools-1.5.5-ui-test.vsix.zip

Tatsinnit
Tatsinnit previously approved these changes Feb 13, 2025
Copy link
Member

@Tatsinnit Tatsinnit left a comment

Choose a reason for hiding this comment

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

❤️ Thank you so much for this, all these changes looks good as they are amorist the deprecation of u/x tag incremental PR, I hope you have done a quick pass for all the changes.

Thanks heaps.

@tejhan
Copy link
Collaborator Author

tejhan commented Feb 14, 2025

Hey all, just pushed committed some recent changes. The remaining elements from the initial commits have been completely wiped, alongside radio group, radio inputs, & textfield inputs.

The remaining elements like the dropdown buttons & panel tabs will have to be replaced by custom JS/React elements that can still mimic the structural functionality we already have, so I'm working on these now & they will be changed in the next incremental deprecation PR.

Newest v6:
vscode-aks-tools-1.5.5-ui-test.vsix.zip

Copy link
Member

@Tatsinnit Tatsinnit left a comment

Choose a reason for hiding this comment

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

❤️ Nice work on this! 🎉 really appreciate the effort that went into this update.

One thought—would it make sense to break this down into feature-specific PRs? That way, if a bug pops up, it would only affect one or two commands rather than the entire set. Having a single PR with 44 files could make it tricky to track down issues if something doesn't work as expected.

Alternatively, we could do a smaller PR first to validate the core changes and follow up with a larger one once we confirm most of the deprecations work smoothly. What do you think? 😊

Fyi and thought share with @hsubramanianaks , @ReinierCC , @qpetraroia

@ReinierCC
Copy link
Collaborator

❤️ Nice work on this! 🎉 really appreciate the effort that went into this update.

One thought—would it make sense to break this down into feature-specific PRs? That way, if a bug pops up, it would only affect one or two commands rather than the entire set. Having a single PR with 44 files could make it tricky to track down issues if something doesn't work as expected.

Alternatively, we could do a smaller PR first to validate the core changes and follow up with a larger one once we confirm most of the deprecations work smoothly. What do you think? 😊

Fyi and thought share with @hsubramanianaks , @ReinierCC , @qpetraroia

I agree. Maybe we could also set up a doc to check off and distrubute which pages have been tested.

Copy link
Collaborator

@hsubramanianaks hsubramanianaks left a comment

Choose a reason for hiding this comment

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

LGTM, we can either split few pages among us for testing and make sure it is tested in all platforms or we can separate it out based on some functionalities or usage based (4 PR's?? ) . I know kubectl run command, ASO, InspecktorGagdet are used in regular, we can do that as separate PR. Also CreateCluster and CreateFleet can be a separate one. Retina and tcpDump can be combined since they are similar.

This reminds me that we should have some automated testing for webview and extension functionalities so that we don't have to completely rely on Manual testing :).. Let's plan for it in the future. Thanks Tejhan.

@tejhan
Copy link
Collaborator Author

tejhan commented Feb 19, 2025

LGTM, we can either split few pages among us for testing and make sure it is tested in all platforms or we can separate it out based on some functionalities or usage based (4 PR's?? ) . I know kubectl run command, ASO, InspecktorGagdet are used in regular, we can do that as separate PR. Also CreateCluster and CreateFleet can be a separate one. Retina and tcpDump can be combined since they are similar.

This reminds me that we should have some automated testing for webview and extension functionalities so that we don't have to completely rely on Manual testing :).. Let's plan for it in the future. Thanks Tejhan.

Regarding the comments, good points, I will go ahead and break this down into smaller PR's. I can group related pages together. I'm wrapping up the final PR for deprecation which would be similarly large but I think I'll just break that up as well, so each can be quickly looked at and reviewed. I've tested out each change extensively myself but for the larger team to test it would be easier to split things up.

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.

4 participants