Update Loading and Summary pages for target configuration to look similar to local device configuration flow #2527
Conversation
krschau
changed the title
|
One thing I noticed in your screenshots is this: I believe the current local experience makes the repositories cloned and apps installed closer to the center. I think we want the same experience here. also for the clone repository scenario did we want to do the same string splitting thing we do for the install case? Potentially maybe this image is of an older build? |
tools/SetupFlow/DevHome.SetupFlow/Services/ConfigurationFileBuilder.cs
Outdated
Show resolved
Hide resolved
tools/SetupFlow/DevHome.SetupFlow/ViewModels/ConfigurationUnitResultViewModel.cs
Show resolved
Hide resolved
tools/SetupFlow/DevHome.SetupFlow/ViewModels/ConfigurationUnitResultViewModel.cs
Show resolved
Hide resolved
|
Can you please run a local configuration file to ensure it's working as expected? |
| @@ -180,22 +184,22 @@ private WinGetConfigResource CreateResourceFromTaskForWinGetDsc(InstallPackageTa | |||
| { | |||
| // WinGet configure uses the Id property to uniquely identify a resource and also to display the resource status in the UI. | |||
| // So we add a description to the Id to make it more readable in the UI. These do not need to be localized. | |||
| id = $"{arguments.PackageId} | Install: " + task.PackageName; | |||
| id = $"{arguments.PackageId}{PackageNameSeparator}{task.PackageName}"; | |||
There was a problem hiding this comment.
Just wanted to double check if this works.
The line dependsOn : Git.Git | Install: Git is actually causing tasks to fail for Dev Boxes.
There was a problem hiding this comment.
I am not sure what the question is. The change in this line is only replacing "| Install:" with a constant to use it in parsing when we receive results. There is no change in functionality. And yes, it works for local and Hyper-V setup. In both cases we send unchanged YAML file we generate here to WinGet. If DevBox doesn't work we'll need to understand if they do anything with the YAML before sending to WinGet.
There was a problem hiding this comment.
Yeah, I don't mean this line in general but the whole dependsOn. FYI - It doesn't work for DevBox (and as a work around we will remove it during parsing.)
| @@ -113,72 +115,237 @@ | |||
| x:Uid="ms-resource:///DevHome.SetupFlow/Resources/SummaryPage_HeaderWithErrors"/> | |||
| </Grid> | |||
|
|
|||
| <Grid Grid.Row="2" ColumnSpacing="25" Visibility="{x:Bind ViewModel.ShowConfigurationUnitResults, Mode=OneWay}"> | |||
| <Grid Grid.Row="2" ColumnSpacing="50" Visibility="{x:Bind ViewModel.ShowConfigurationUnitResults, Mode=OneWay}"> | |||
There was a problem hiding this comment.
Did you have a chance to review these changes for accessibility issues?
There was a problem hiding this comment.
This is the same value as for local case below. I assume the local setup was reviewed when it was introduced.
This whole change duplicates most of the XAML used for the local setup with replacement of variables that needed for target setup.
There was a problem hiding this comment.
I would recommend changing the text size from Settings -> Accessibility and checking once though.
|
Just pulled down this branch and tested it with this DSC config file: https://github.com/microsoft/devhome/blob/main/docs/sampleConfigurations/DscResources/GitDsc/CloneWingetRepository.yaml Here is what the results look like with Dev Home's april release last month (0.12): Dev.Home.april.release.Winget.Git.clone.mp4and now here is what it looks like with you're change. (Although there could always be some other change that went in after that release that could be affecting the margins etc, but the summary page no longer shows the old configuration results which I think this change likely affects. Dev.Home.Dev.with.Sergeys.change.Winget.Git.clone.mp4 |
|
I didn't know that the same view was used in that scenario, so yes, I broke it. I'll see if these changes can be implemented without breaking that scenario. |
There was a problem hiding this comment.
@bbonaby thanks for helping with testing local DSC. Requesting change to ensure that flow is working as expected.
Fix incorrect merging.
|
Last commit fixed Summery page for setup using a configuration file and incorrect merging. |
AmelBawa-msft
requested review from
AmelBawa-msft
and removed request for
AmelBawa-msft
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Summary of the pull request
Note: In future changes we should consider separating local and target case views to be independent or refactor data structures to better accommodate both cases. It becomes hard and cumbersome to maintain this code with risk of regressing one of the flows while making changes to another one.
Validation steps performed
Tested UX running configuration flow to configure a Hyper-V VM from the host device.
PR checklist