Azure / aks-devx-tools

Create deployment files and configure GitHub Actions workflows to deploy applications to Azure Kubernetes Service (AKS).
MIT License
17 stars 17 forks source link

Swap to vscode toolkit for Draft #10

Closed OliverMKing closed 2 years ago

OliverMKing commented 2 years ago

Description

Swaps the UI to use VS-code toolkit UI to look more integrated and be more accessible.

Features:

Old UI

old-draft old-dark old-red

New UI

Screen Shot 2022-09-13 at 5 04 02 PM Screen Shot 2022-09-13 at 5 04 13 PM

Hides fields that don't do anything for docker option.

Screen Shot 2022-09-13 at 5 05 12 PM

Shows missing fields

Screen Shot 2022-09-13 at 5 05 29 PM Screen Shot 2022-09-13 at 5 05 46 PM Screen Shot 2022-09-13 at 5 06 13 PM

Theming support

Type of change

Please delete options that are not relevant.

How Has This Been Tested?

Tested manually with all scenarios

Checklist:

Tatsinnit commented 2 years ago

šŸ’” Please note build is failing because @vscode/webview-ui-toolkit probably has deep dependency on @microsoft/fast-react-wrapper@0.1.48 which intern needs react package hence error: react@>=16.9.0, required by @microsoft/fast-react-wrapper@0.1.48

https://www.npmjs.com/package/react/v/16.9.0

Only interesting thing is that npm install should have done that, so not sure what happened, might be good to verify that it is something missed dependency within the parent package.


webpack 5.73.0 compiled successfully in 19174 ms
Error: Command failed: npm list --production --parseable --depth=99999 --loglevel=error
npm ERR! peer dep missing: react@>=16.9.0, required by @microsoft/fast-react-wrapper@0.1.48

Error: Process completed with exit code 1.
karenychen commented 2 years ago

@OliverMKing Great job on the UI work! I was wondering if we plan to include the UI changes for the other draft commands in this PR?

OliverMKing commented 2 years ago

@OliverMKing Great job on the UI work! I was wondering if we plan to include the UI changes for the other draft commands in this PR?

Probably not on this PR but I think we should wait until they are done until we create a new release.

OliverMKing commented 2 years ago

šŸ’” Please note build is failing because @vscode/webview-ui-toolkit probably has deep dependency on @microsoft/fast-react-wrapper@0.1.48 which intern needs react package hence error: react@>=16.9.0, required by @microsoft/fast-react-wrapper@0.1.48

https://www.npmjs.com/package/react/v/16.9.0

Only interesting thing is that npm install should have done that, so not sure what happened, might be good to verify that it is something missed dependency within the parent package.

webpack 5.73.0 compiled successfully in 19174 ms
Error: Command failed: npm list --production --parseable --depth=99999 --loglevel=error
npm ERR! peer dep missing: react@>=16.9.0, required by @microsoft/fast-react-wrapper@0.1.48

Error: Process completed with exit code 1.

https://github.com/microsoft/vscode-webview-ui-toolkit/issues/386 Interesting related issue

karenychen commented 2 years ago

Tested the UI to be working correctly on my machine āœ… For the deployment only option, the UI prompts the user to specify the language field but i believe that is only needed for dockerfile creation. Since we are hiding unnecessary fields could we also hide the language selector when it is deployment only?

OliverMKing commented 2 years ago

Tested the UI to be working correctly on my machine āœ… For the deployment only option, the UI prompts the user to specify the language field but i believe that is only needed for dockerfile creation. Since we are hiding unnecessary fields could we also hide the language selector when it is deployment only?

Updated with that šŸ˜„.

OliverMKing commented 2 years ago

This PR will break the CSS of the other scenarios right now.

The goal should be full parity with whatever UX option we choose so if we decide to stick with an upgraded webview scenario like this we will migrate them all to this which fixes that issue. Going to hold off on merging for now until we make a decision around the UX approach we want to go with.

OliverMKing commented 2 years ago

There is parity missing with this, need windows vs Mac test as well, thanks.

@Tatsinnit did you find something different between mac and windows? There shouldn't be any differences. Not sure what this comment means.

Tatsinnit commented 2 years ago

There is parity missing with this, need windows vs Mac test as well, thanks.

@Tatsinnit did you find something different between mac and windows? There shouldn't be any differences. Not sure what this comment means.

Cool, so I think internally we discussed @OliverMKing, also @sabbour did mentioned windows UI broken and here is from my windows machine. Hope this helps. Thanks.

image
OliverMKing commented 2 years ago

Going to close this for now. Seems like we are going in a different direction.