Pager: Add Auto Display Mode functionality #3239

gvazquez8 · 2020-09-03T21:07:06Z

Previously, Auto Display mode only set the pager control to the ComboBox Display mode.
This PR allows the Auto Display mode to decide between the ComboBox display mode and NumberBox Display mode based on the NumberOfPages.

Motivation and Context

Proposal: #60
Spec: link

How Has This Been Tested?

Manually tested as well as Interaction Tests

Screenshots (if appropriate):

…ous changes to PrototypePager.cs

…mily on pager buttons Segoe MDL2 static resource

…thin OnApplyTemplate

Removing c++ project

Removing C++ project

…ge and codebehind to use it.

This reverts commit 35ee782.

This reverts commit 9665cea.

This reverts commit fadef53.

gvazquez8 · 2020-09-03T21:07:50Z

/azp run

azure-pipelines · 2020-09-03T21:08:06Z

Azure Pipelines successfully started running 1 pipeline(s).

marcelwgn · 2020-09-03T21:10:20Z

dev/Pager/InteractionTests/PagerTests.cs

@@ -812,6 +823,19 @@ void VerifyDisplayMode(DisplayModes mode)
            switch (mode)
            {
                case DisplayModes.Auto:
+                    if (elements.GetNumberOfPagesTextBlock().GetText() == "100")


I think it might be better to make this a bit more flexible, e.g. try to convert this to int and check if its higher than lets say 50.

Or ideally use the magic const 11 we use in the actual control.

marcelwgn · 2020-09-03T21:14:51Z

dev/Pager/PrototypePager/PrototypePager.Events.cs

@@ -113,10 +114,12 @@ private void OnPagerDisplayModeChanged()
        {
            switch (this.PagerDisplayMode)
            {
+                case PagerDisplayModes.Auto:
+                    VisualStateManager.GoToState(this, NumberOfPages < 11 ? ComboBoxVisibleVisualState : NumberBoxVisibleVisualState, false);


@MarissaMatt The proposal only says something about less than 11 and larger than 11, what about 11 itsself? Is case 11 resulting in NumberBox correct here? I think the spec might need some clarification for this.

@chingucoding can you point me to where it talks about the "less than 11 and larger than 11" on the proposal? I can't seem to find any reference to it there. I know the spec mentions it, but I'm interested to see what the proposal mentions.

Sure, it is the first paragraph in this section: https://github.com/microsoft/microsoft-ui-xaml-specs/blob/user/savoyschuler/pagercontrol/active/PagerControl/PagerControl.md#creating-a-pager-control

It states:

"[...] NumberOfPages property is less than 11 and will show the number box mode if it is greater than 11."

I updated the spec with that requirement but not the proposal so I will make sure they are both matching. The requirement should be if it's 10 pages or less then combo box will be chosen and if it's 10 or more then number box will be chosen.

I'm still a little confused..

So should it be:
if NumberOfPages <= 10 then use ComboBox else use NumberBox
or
if NumberOfPages < 10 then use ComboBox else use NumberBox

It should be if NumberOfPages < 10 then use ComboBox else use NumberBox

Great thanks! I'll update the prototype

gvazquez8 · 2020-09-03T23:24:13Z

/azp run

azure-pipelines · 2020-09-03T23:24:29Z

Azure Pipelines successfully started running 1 pipeline(s).

marcelwgn · 2020-09-03T23:47:15Z

dev/Pager/PrototypePager/PrototypePager.cs

@@ -33,10 +33,11 @@ public enum ButtonVisibilityMode { Auto, AlwaysVisible, HiddenOnEdge, None, }
        private ItemsRepeater PagerNumberPanel;
        private Rectangle NumberPanelCurrentPageIdentifier;

+        private static int AutoDisplayModeNumberOfPagesThreshold = 10; // This integer determines when to switch between NumberBoxDisplayMode and ComboBoxDisplayMode 


Just putting this out as a thought: Should we make this customizable? Of course not in this PR, but maybe this is something we could think about for the future.

I don't think this adds very much, if you want to customize this you could just change your display mode from combo box to numberbox at your chosen threshold, which doesn't seem like an issue.

Fair point, though with that argument you could also argue why we need this in the first place given that devs can implement it themselves. But yeah, it probably doesn't add much to the API.

But this gets us the default behavior that our designers think is best. If another app's designer wants a different behavior they can change that but if they want to default to our design this does that for them

In reply to: 483778517 [](ancestors = 483778517)

Oh yes, that's a very good point. That ensures that app's all use the recommended behavior if they aren't actively trying to work around that. This shouldn't be customizable then.

StephenLPeters · 2020-09-04T18:29:36Z

dev/Pager/PrototypePager/PrototypePager.TestHooks.cs

@@ -14,5 +14,7 @@ public sealed partial class PrototypePager : Control
        public ComboBox ComboBoxDisplayTestHook;
        public ItemsRepeater NumberPanelDisplayTestHook;
        public Rectangle NumberPanelCurrentPageIdentifierTestHook;
+
+        public int AutoDisplayModeNumberOfPagesThresholdTestHook;


AutoDisplayModeNumberOfPagesThresholdTestHook [](start = 19, length = 45)

It looks like your test app doesn't use this expect to show the value. In a production environment these are not free and I think we should avoid them if they aren't useful. For the production version of this I would just hard code the value in the test app.

I've removed it!

StephenLPeters · 2020-09-04T18:31:02Z

dev/Pager/InteractionTests/PagerTests.cs

@@ -529,6 +529,17 @@ public void ChangingDisplayModeTest()

                SetNumberPanelDisplayMode();
                VerifyNumberPanelDisplayMode();
+
+                ChangeNumberOfPages();
+                VerifyNumberOfPages("100");


100 [](start = 37, length = 3)

I would also verify the boundaries, since the display mode changes at 10 we should be testing 9 and 10 as well.

gvazquez8 · 2020-09-04T19:34:34Z

/azp run

azure-pipelines · 2020-09-04T19:34:49Z

Azure Pipelines successfully started running 1 pipeline(s).

Gabriel Vazquez and others added 30 commits July 6, 2020 17:10

Initial setup for Pager

089f8c0

Initial Build for MUXControlsInnerLoop

88cf084

Added Pager Prototype for NumberBox & ComboBox

aaac224

Added Pager Interaction Tests for Edge Buttons and Display Modes.

6e45803

Delete unnecessary file

6cb0cf4

Updated InnerLoop files

27b8f07

Removed unnecessary statements, added comment.

6dbb145

Fixed Conflicts with master

3742723

Added Test Suites to Pager Tests

44908d8

Refactored tests, removed redundant tests, removed C++ Pager and vari…

0118bb3

…ous changes to PrototypePager.cs

Undo small change from last commit and added better names to xaml

1561e7d

Merge branch 'master' into master

d1d95f0

Removed some converters

2c8c8a6

Merge branch 'master' of https://github.com/gvazquez8/microsoft-ui-xaml

e01dd1a

Added button width resource for NumberPanel mode

b31a814

Updated visual state variable names in codebehind and updated font fa…

0c5a85b

…mily on pager buttons Segoe MDL2 static resource

Made property changed events non static and attached to properties wi…

2e8cfdc

…thin OnApplyTemplate

Removed unnecessary setters in visual states

40828c1

Delete Pager.properties.cpp

f562f14

Removing c++ project

Delete Pager.properties.h

5aebd68

Removing c++ project

Delete Pager.cpp

8216c1b

Removing C++ project

Delete Pager.h

b210a0d

Delete Pager.idl

fadef53

Delete Pager.vcxitems

35ee782

Delete Pager.xaml

9665cea

Added PreviousPage parameter to PageChangedEventArgs. Adjusted TestPa…

089c575

…ge and codebehind to use it.

Merge branch 'master' of https://github.com/gvazquez8/microsoft-ui-xaml

a711afa

Revert "Delete Pager.vcxitems"

ac2e16a

This reverts commit 35ee782.

Revert "Delete Pager.xaml"

ed59b07

This reverts commit 9665cea.

Revert "Delete Pager.idl"

11cebd3

This reverts commit fadef53.

gvazquez8 added 6 commits August 24, 2020 16:38

Merge https://github.com/microsoft/microsoft-ui-xaml

6a053e3

Merge https://github.com/microsoft/microsoft-ui-xaml

cf87ea4

Added Auto mode behavior.

059b0f4

Merge https://github.com/microsoft/microsoft-ui-xaml

924e980

Merge in master

8121b28

Modified auto display mode tests

409172c

msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Sep 3, 2020

gvazquez8 requested a review from StephenLPeters September 3, 2020 21:07

marcelwgn reviewed Sep 3, 2020

View reviewed changes

StephenLPeters added area-Pager team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Sep 3, 2020

Changed threshold value for auto display mode and removed magic numbers.

4c3312a

marcelwgn reviewed Sep 3, 2020

View reviewed changes

marcelwgn approved these changes Sep 4, 2020

View reviewed changes

StephenLPeters reviewed Sep 4, 2020

View reviewed changes

Added test to check auto display mode threshold boundry

b655165

gvazquez8 requested a review from StephenLPeters September 4, 2020 19:42

StephenLPeters approved these changes Sep 4, 2020

View reviewed changes

gvazquez8 merged commit a154532 into microsoft:master Sep 4, 2020

gvazquez8 deleted the AutoDisplayModeDev branch September 4, 2020 22:45

marcelwgn mentioned this pull request Sep 4, 2020

User/savoyschuler/pagercontrol microsoft/microsoft-ui-xaml-specs#88

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pager: Add Auto Display Mode functionality #3239

Pager: Add Auto Display Mode functionality #3239

gvazquez8 commented Sep 3, 2020 •

edited

Loading

gvazquez8 commented Sep 3, 2020

azure-pipelines bot commented Sep 3, 2020

marcelwgn Sep 3, 2020

marcelwgn Sep 3, 2020

marcelwgn Sep 3, 2020

gvazquez8 Sep 3, 2020

marcelwgn Sep 3, 2020

MarissaMatt Sep 3, 2020

gvazquez8 Sep 3, 2020

MarissaMatt Sep 3, 2020

gvazquez8 Sep 3, 2020

gvazquez8 commented Sep 3, 2020

azure-pipelines bot commented Sep 3, 2020

marcelwgn Sep 3, 2020

StephenLPeters Sep 4, 2020

marcelwgn Sep 4, 2020

StephenLPeters Sep 4, 2020

marcelwgn Sep 4, 2020

StephenLPeters Sep 4, 2020

gvazquez8 Sep 4, 2020

StephenLPeters Sep 4, 2020

gvazquez8 Sep 4, 2020

gvazquez8 commented Sep 4, 2020

azure-pipelines bot commented Sep 4, 2020

Pager: Add Auto Display Mode functionality #3239

Pager: Add Auto Display Mode functionality #3239

Conversation

gvazquez8 commented Sep 3, 2020 • edited Loading

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

gvazquez8 commented Sep 3, 2020

azure-pipelines bot commented Sep 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvazquez8 commented Sep 3, 2020

azure-pipelines bot commented Sep 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvazquez8 commented Sep 4, 2020

azure-pipelines bot commented Sep 4, 2020

gvazquez8 commented Sep 3, 2020 •

edited

Loading