microsoft / TemplateStudio

Template Studio accelerates the creation of new WinUI 3, WPF, and UWP apps using a wizard-based experience.
Other
2.67k stars 457 forks source link

Adapt CaliburnMicro's Master-Detail page to new Master-Detail design #1184

Closed sibille closed 6 years ago

sibille commented 6 years ago

We made enhancements to the master-detail page to adjusts its design to master detail redlines. CaliburnMicro is still using the old design, we should adapt to the new one.

sibille commented 6 years ago

Caliburn.Micro's Master Detail designs updated, pending revision in #1183. Caliburn.Micro uses control from UWP Community Toolkit, should we change too? I remember we had a discussion on this, but unsure about the outcome.

mrlacey commented 6 years ago

Can you find the past discussion? I think we didn't want to rebuild for the sake of it. We should use the same for all frameworks.

sibille commented 6 years ago

Found it!! https://github.com/Microsoft/WindowsTemplateStudio/issues/157 @crutkas, what do you think?

mrlacey commented 6 years ago

It seems there wasn't a good enough reason to make the move previously. Have things changed? What are the implications for changing now?

ralarcon commented 6 years ago

The thing is that the Caliburn Master / Detail template is using the toolkit, so we need to align in one sense or the other.

mvegaca commented 6 years ago

I vote for using toolkit control in all frameworks.

mrlacey commented 6 years ago

This isn't just a straight-up vote. justify decisions either way.

mvegaca commented 6 years ago

If we are now using toolkit on SplitView, could be a good idea to use it on MasterDetail and reduce the generated code.

crutkas commented 6 years ago

My fear with that control is we abstract away too much. We had that issue open for a long time and no community member really pushed for it.

At this point also, why would we shift. Does the new image gallery style page use this control?

The reason why we took the split view was so we could have an easy transition to Nav view control once the next update of Windows is released post Fall Creators Update.

ralarcon commented 6 years ago

The image gallery do not use the toolkit control as we didn't see any advantage on using it.

From my point of view we can live with the fact of having the Caliburn.Micro template based on the toolkit and the others not. At the same time, I don't now how mandatory is to have all the templates really similar (as much as possible), considering Caliburn has more differentiations than the others.

I think we can leave the things as they are right now (refactorizing the sytles and improving what is needed).

crutkas commented 6 years ago

Wait wait wait. All code generated should be basically the same across frameworks and project types.

Why is one master detail not like the others

ralarcon commented 6 years ago

Basically because Caliburn.Micro master-detail template use the toolkit. We didn't found the differencies but understood the templates implementation were already checked.

@nigel-sampson is there any reason behind the scenes we can't see to use the toolkit instead of being aligned with what we currently have? If not, we need to align both implementations. In fact there are other splited pages (Map, Settings, Tabbed and WebView) so we understood there was a really framework dependant implementations for these pages.

crutkas commented 6 years ago

Can we go through and validate then and be 100% sure xaml is basically the same. We should be generating the same stuff when possible. If everyone wants to migrate toward the control, I am fine with that but I want to stabilize.

mvegaca commented 6 years ago

I'm working on adapt FCU redlines on MasterDetail and reviewing the styles as all were hardcoded in xaml. I'm going to take the chance to review the impact of using UWP toolkit control. At a first glance, it looks more simple.

Redlines

image

Current

image

nigel-sampson commented 6 years ago

Apologies for not seeing the #157 discussion, I ended up using the toolkit control for most of the pro's discussed here and in the other thread.

I felt the view model in the non control version became too complicated in that most of it was implementing view concerns around state and not actually being a view model. This is probably a separate discussion since that covers a number of templates.

Also using that control helps highlight the view locator features of Caliburn.Micro, it's actually implementation is pretty light, so shouldn't have too much of an impact on design.

Most of the design changes for Caliburn.Micro would be in the MasterView and DetailView, if there are design changes to be made there before release I'm happy to make them.

mvegaca commented 6 years ago

Hi @nigel-sampson I'm working on this branch applying the redlines to Master Detail Page. I've added UWP Toolkit MasterDetail control to MVVM Basic, MVVM Light and CodeBehind Frameworks. I also added some generation updates to caliburn micro files to be aligned with other frameworks.

If you want to see more, I added the branch url. https://github.com/mvegaca/WindowsTemplateStudio/tree/Issue1184-redlines

I've not add the PR jet because I need to fix template tests and do some refactoring.

nigel-sampson commented 6 years ago

Ok, I'll have a look over it later, but at first glance I'm not sure the renaming of the child views is required.

The idea with CM is that you can have multiple views for the same view model based on context, but they're in their own sub folder so should probably stay as MasterView and DetailsView for consistency across multiple uses of the template.

mvegaca commented 6 years ago

The idea is having different definitions of listview template and detail content with names related to the selected name. If a user wants to add two different MasterDetail pages (i.e. SportNews, EconomyNews) then he can define a different list and detail layout for both pages.

mvegaca commented 6 years ago

There is one thing that I'm not reaching to do on Caliburn.Micro.

MasterDetail LoadDataAsync()

public async Task LoadDataAsync()
{
   Items.Clear();

   var data = await SampleDataService.GetSampleModelDataAsync();
   Items.AddRange(data.Select(d => new MartinDetailViewModel(d)));
// Do this only if current VisualState == Wide
   ActiveItem = Items.First();
// End IF
}

To implement that IF sentence we need to access VisualStateManager from Page. I don't know what is the best approach to access visual state manager current state from ViewModel in Caliburn.Micro.

Could you please help me with that @nigel-sampson ?

mvegaca commented 6 years ago

Finally, on this commit I've checked MasterDetailsViewControl.ViewState == MasterDetailsViewState.Both to decide if initialize Selected item.

mvegaca commented 6 years ago

Add redlines and aligned code between Caliburn.Micro and other frameworks #1222.

Is necessary to add if sentence in Caliburn.Micro to check MasterDetailsViewControl.ViewState == MasterDetailsViewState.Both

nigel-sampson commented 6 years ago

@mvegaca

If they have two different master details they'll be under /Views/SportsNews/MasterView.xaml and /Views/Economy/MasterView.xaml so there will be no conflict.

mvegaca commented 6 years ago

@nigel-sampson Review required on #1227.

targeting as can close out soon

sibille commented 6 years ago

Verified on dev-nightly: Wizard: 0.14.17284.65001 Templates: 0.14.17284.2