Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User/savoyschuler/pagercontrol #88

Draft
wants to merge 50 commits into
base: master
Choose a base branch
from

Conversation

MarissaMatt
Copy link
Contributor

Initial PR for adding PagerControl spec to master.

Copy link
Contributor

@marcelwgn marcelwgn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While generally, the numbers are shown at the bottom of the control, should we also allow to render them at the the top? Or does the PagerControl not actually render but rather provides the small "bar" at the bottom of the screenshots?

active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Show resolved Hide resolved
active/PagerControl/PagerControl.md Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
MarissaMatt and others added 5 commits July 2, 2020 11:36
Co-authored-by: MikeHillberg <18429489+MikeHillberg@users.noreply.github.com>
Co-authored-by: MikeHillberg <18429489+MikeHillberg@users.noreply.github.com>
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
Co-authored-by: MikeHillberg <18429489+MikeHillberg@users.noreply.github.com>
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved
active/PagerControl/PagerControl.md Outdated Show resolved Hide resolved

private async void OnPageLoaded(object sender, RoutedEventArgs e)
{
imageList = await GetImageList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per an earlier comment, I think the primary scenario for this control is data virtualization. Towards that end, to make this sample more realistic, it shouldn't show the images all being pre-fetched; it should show a call to fetch images when the page changes. And then to continue the realism, it needs to show an indeterminate progress bar during the async call to fetch the images from a server?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MikeHillberg in the Pager_PageChanged method we do call GetPageImages to update the images being shown when the page number changes. OnPageLoaded is only called on the initial load. Am I understanding your comment correctly? Are you saying that the imageList = await GetImageList(); should not be included and the images should be retrieved adhoc?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. We could just set the source to a set of images fetched from somewhere based on the page number. That could be a web call for example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarissaMatt - Right, in PageChanged its extracting some images. But in PageLoaded it's getting all of the images ready, probably downloading them all from a server. Pre-downloading all the images like that defeat the purpose of the pager, you might as well put them in a ScrollViewer.

Copy link

@ranjeshj ranjeshj Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the sample in the previous comment to do an async call with just the images fetched for each page. We should add this full sample to XamlControlsGallery and link it in the docs.


# API Details

```c++
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks more like IDL than C++.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MikeHillberg I was following the template for this section. Is there something I should update this to?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically only for GitHub (and other MarkDown render engines) to know what syntax highlighting to use, but it also helps when reading the raw md file to know what language this is written in. If the API details are in (M)IDL format, this should be IDL. I'm a bit surprised that C++ is in the template, usually APIs are done in IDL as we can simply copy paste that into the actual WinUI source code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's following the template, except that it needs to be IDL. Changing this to Int32 might be the only issue

Comment on lines 251 to 252
Integer NewPageIndex{get; };
Integer PreviousPageIndex{get; };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is IDL, this would be Int32.

ButtonPanel,
};

enum PagerButtonVisibility
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the control is named PagerControl, would it be better to name this PagerControlButtonVisibility?


}

runtimeclass PagerControl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how that snuck in, but now we have two lines defining the PagerControl class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

| Name | Description| Default |
|:---:|:---|:---|
| PagerDisplayMode | Enum that contains 4 values (Auto, ComboBox, NumberBox, ButtonPanel) to represent the look of the pager control. When Auto is selected, the display mode will be ComboBox if the NumberOfPages property is less than 11 otherwise it will be NumberBox. | Auto |
| PagerButtonVisibilityBehavior | Enum that contains 4 values (Visible, HiddenOnEdge, Hidden) that allows the app developer to hide or show the four edge buttons. HiddenOnEdge will remove the appropriate buttons if the selected page is the last or first page. When the last page is selected, the next and last buttons will be disabled and same when the first page is selected for the first and previous page. When Auto is selected, the visibility mode will be AlwaysVisible. | Visible |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are only 3 values now, we removed Auto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

| First, Previous, Next, and Last ButtonCommand | Specially handle the button pressed event for when the end user selects the buttons. | N/A |
| First, Previous, Next, and Last Style | Give the developer the option to customize the style by changing the text or glyph for the edge buttons.| N/A |
| ButtonPanelAlwaysShowFirstAndLastPage | Note: This property only applies to the button panel display mode.Boolean to display the ellipses and the first and last index of the numerical button panel display mode. | True |
| SelectedIndex | The 0 based index that is currently selected. It will default to the first index. | 0 |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IDL code says that this is SelectedPageIndex, is that correct, and this entry wasn't updated? Or should we stick with SelectedIndex?

Also, just to clarify, when using this control in ComboBox, when the user clicks on entry "2", the PagerControl will report it's SelectedPageIndex as 1 right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is correct! I also updated the name so it will say SelectedPageIndex.

@marcelwgn
Copy link
Contributor

Would it make sense to expose a property to modify when the NumberPanel starts working with ellipsis? In some cases, developers might have place for 20 numbers and in other cases, developers might only want to have 4 buttons and most and otherwise use ellipsis. Currently, we would need to hardcode that magic number without letting users modify that.

@marcelwgn
Copy link
Contributor

Also, currently the indicator color is hardcoded into the template. Would it be better to use the users accent color or expose a theme resource for that?

@Felix-Dev
Copy link

Felix-Dev commented Sep 23, 2020

Also, currently the indicator color is hardcoded into the template. Would it be better to use the users accent color or expose a theme resource for that?

Just like with the NavigationView's selection indicator, a theme resource should be introduced and the default color value should be the user accent color. The theme resource could be named PagerControlSelectionIndicatorForeground to be similar to the NavigationView's selection indicator resource which is named NavigationViewSelectionIndicatorForeground.

# API Notes
| Name | Description| Default |
|:---:|:---|:---|
| PagerControlDisplayMode | Enum that contains 4 values (Auto, ComboBox, NumberBox, ButtonPanel) to represent the look of the pager control. When Auto is selected, the display mode will be ComboBox if the NumberOfPages property is less than 11 otherwise it will be NumberBox. | Auto |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ButtonPanel --> NumericalButtonPanel to match above.

Would love to add BreadbrumbButtonPanel here in the future for the "row of dots" style.

@marcelwgn
Copy link
Contributor

@MarissaMatt @SavoySchuler @MikeHillberg What is the status of this PR? Is this ready for "review review"?

@SavoySchuler
Copy link
Member

@gabbybilka are you the current owner of this while @MarissaMatt is on leave? Asking per @chingucoding's question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants