Refactor ItemPage and content pages

[Jay-o-Way]

Situation

The current structure of ItemPage and all the "specific" pages is...interesting?

<!-- ItemsPage.xaml -->
<Grid>
 <controls:PageHeader ...>
 <ScrollViewer name ="svPanel" ...>
  <Grid name="contentRoot" Padding="36,0,36,36">
   <TextBlock x:Name="descriptionText" ...>
   <Frame x:Name="contentFrame" Grid.Row="1" />

Note that contentRoot Padding disregards the fact that a page may have a scroll viewer, which must be able to stretch to the sides. And then ItemPage.xaml.cs has the following comment:

WinUI-Gallery/WinUIGallery/Pages/ItemPage.xaml.cs

Lines 77 to 78 in 297974e

	// Disabling page scrolling, as the page itself will have ScrollViewers to handle specific scrolling use cases
	svPanel.VerticalScrollBarVisibility = ScrollBarVisibility.Disabled;

The ScrollViewer should be moved out of ItemPage.xaml to the content pages of contentFrame. I know this will be a lot of work, but it really is for the best.

Suggestions

ItemPage.xaml:

<Page>
    <Grid x:Name="rootPanel">
        <Grid.RowDefinitions>
            <RowDefinition Height="Auto" />
            <RowDefinition Height="Auto" />
            <RowDefinition Height="*" />
        </Grid.RowDefinitions>
       <controls:PageHeader
           x:Name="pageHeader"
           Margin="36,24,36,8"
           Item="{x:Bind Item}" />

        <!-- see comment below in discussion: why not move this to each page? -->
        <TextBlock
            x:Name="descriptionText"
            Grid.Row="1"
            Margin="36,0,36,8"
            Text="{x:Bind Item.Description}"
            TextWrapping="Wrap"
            Visibility="{x:Bind Item.Description, Converter={StaticResource stringVisibilityConverter}}" />

        <Frame
            x:Name="contentFrame"
            Grid.Row="2" />

Style:

<!-- App.xaml -->
<Thickness x:Key="ContentPageBaseMargin">...</Thickness>

On every Page:

<Page>
    <!-- optional description here -->
    <ScrollViewer>
        <StackPanel Margin="{StaticResource ContentPageBaseMargin}">
        <!-- or -->
        <Grid  Margin="{StaticResource ContentPageBaseMargin}">

Note that some pages, like Iconography, already deal with scrolling as-is.

[Jay-o-Way]

Proof of concept: This seems to work well. Only adapted pages Color/Text, ButtonPage and IconographyPage for simplicity.

https://github.com/Jay-o-Way/WinUI-Gallery/tree/refactor-ItemPage

Note

At the moment, the change means <TextBlock x:Name="descriptionText" ...> falls outside of the ScrollViewer. Also note that the use of this textblock is inconsistent. Four out of five pages under "Design" have a description on the Page itself. This makes me think it any description might as well be "hard-coded" on any page, removing the need for this textblock (in ItemsPage.xaml) and making the description scroll with the rest of the page.

[Jay-o-Way]

FYI @Zakariathr22 @niels9001 @marcelwgn

[niels9001]

I'm trying to understand the main benefit of doing this? Moving scrollviewers to sample pages is a lot of work and I'm a bit worried that it might make things harder to maintain?

[Jay-o-Way]

I'm trying to understand the main benefit of doing this?

Grid contentRoot introduces a Padding that should not be there. Correcting this is complicated, often with negative margins. PR #2027 shows some of this complexity.

Moving scrollviewers to sample pages is a lot of work

I know this will be a lot of work, but it really is for the best. Also note that some pages (e.g. Iconography) use a different layout, so with this change it'll be easy to keep the design of such a page in mind. Currently, it looks like a developer working on a page must be aware of "upper level" scrollviewers and paddings, that are out of scope for a page level.

it might make things harder to maintain?

I don't think so 🤓

[niels9001]

My point is that adding ScrollViewers to every page might introduce other issues, that we then need to fix. Currently, a sample page is just a StackPanel with content and that's the only thing a developer would need to worry about. I rather have as much content in ItemsPage to keep consistency and maintainability.

So what's the main benefit over what we have now? Except for not using negative margins?

[Jay-o-Way]

A few screenshots from my concept (at this moment of writing)
The red border is <frame x:Name="contentFrame">

[niels9001]

On the Button page, that description text should be part of the scrollviewer like we have now - it's not information that should be always visible.

[Jay-o-Way]

I am aware of that issue. I haven't updated the full repository. Yes, At the moment, the change means <TextBlock x:Name="descriptionText" ...> falls outside of the ScrollViewer. But also note that the use of this textblock is inconsistent. Four out of five pages under "Design" have a description on the Page itself. This makes me think any description might as well be "hard-coded" on any page, removing the need for this textblock (in ItemsPage.xaml) and making the description scroll with the rest of the page.