microsoft / microsoft-ui-xaml-specs

API spec repository for the Windows UI Library (WinUI)
https://github.com/Microsoft/microsoft-ui-xaml
Creative Commons Attribution 4.0 International
119 stars 40 forks source link

User/savoyschuler/pagercontrol #88

Open MarissaMatt opened 4 years ago

MarissaMatt commented 4 years ago

Initial PR for adding PagerControl spec to master.

marcelwgn commented 4 years ago

While testing out the current prototype in the MUXControlsTestApp, I noticed the following behavior:

gif

When hiding/showing a button, it gets collapsed and the size of the pager control changes. However this can get problematic with layouting. If users want to center the control for example, and one of the button hides (e.g. because the end was reached and it was set to auto hide), the pager control will start jumping. Even worse, it might actually not look centered anymore if the next and last button are hidden because the width would only take the left buttons and e.g. the NumberBox into account resulting in an off center NumberBox.

The question would be, what is the expected behavior here. Should those buttons still take space even when set to hide?

gvazquez8 commented 4 years ago

@chingucoding I implemented the current behavior because I wanted the control to save as much screen space as it can. That being said, I do agree with your concerns and I think the control should take up as much space in order to avoid this behavior. If we were to do that, it would lead to dead space if there was another ui element to the right or left of the control and one of the buttons hides, right? Would developers mind if this were the case?

marcelwgn commented 4 years ago

Unfortunately, yes that would lead to dead space. I think the million dollar question is, what is better? Have dead space that might create gaps, or an uneven centered control that might create layout jumps when choosing a different page? I would argue that dead space is fine here, especially has these layout jumps can be irritating to end users.

gvazquez8 commented 4 years ago

Yep I agree as well! I'll make some changes to the prototype

Felix-Dev commented 4 years ago

Since the space would normally be occupied by a visible NextPage button and LastPage button, for example, I think its fine to keep the space these buttons would occupy reserved, even if actual buttons might not currently be displayed (if, for example, PagerButtonVisibilityBehavior is set to HiddenOnEdge.) The control should not jump around when these button change their visibility (as that could be quite a frequent occurence) nor should the developer have to account for that when designing the app.

Perhaps we can reserve button space depending on the pager button configuration. For example, when the developer sets the visibility of both the FirstPage button and the LastPage button to none, then we should consider not reserving the space these buttons would occupy. The NumberBox and its PrevPage and NextPage buttons would still be visible so it will look centered and the developer explicitly said they do not want the FirstPage and LastPage buttons to show up. So, if they have custom UI next to the visible PrevPage and NextPage buttons, there should be no additional padding between them enforced by the control here.

marcelwgn commented 4 years ago

Yep I agree as well! I'll make some changes to the prototype

Awesome! Looking forward to that.

marcelwgn commented 4 years ago

Since the space would normally be occupied by a visible NextPage button and LastPage button, for example, I think its fine to keep the space these buttons would occupy reserved, even if actual buttons might not currently be displayed (if, for example, PagerButtonVisibilityBehavior is set to HiddenOnEdge.) The control should not jump around when these button change their visibility (as that could be quite a frequent occurence) nor should the developer have to account for that when designing the app.

Perhaps we can reserve button space depending on the pager button configuration. For example, when the developer sets the visibility of both the FirstPage button and the LastPage button to none, then we should consider not reserving the space these buttons would occupy. The NumberBox and its PrevPage and NextPage buttons would still be visible so it will look centered and the developer explicitly said they do not want the FirstPage and LastPage buttons to show up. So, if they have custom UI next to the visible PrevPage and NextPage buttons, there should be no additional padding between them enforced by the control here.

The issues I see here are a) What if you only set the firstpage visibility to none? Should we then render uneven? b) What if the dev decides that you want to hide the first and last page if there are less than 3 pages for example, and show them when there are 4 or more pages? We would then be again have this jumping behavior. I am not sure how common that scenario is, but I think it's a valid concern.

Felix-Dev commented 4 years ago

@chingucoding

To a) Same as with the current train of thought. If only FirstPage button visibility is set to none (but LastPage button is visible), the space FirstPage button would occupy should be reserved by the Pager control. The assumption here is that if the developer views displaying the LastPage button as useful, then in the majority of scenarios, the FirstPage button will also be visible at least sometimes and not hidden for the entire life time of the full user interaction with the control.

To b) The assumption here is that a UI where no FirstPage button and LastPage buttons are required is "not a rare one" and as such we should not reserve space here. If the developers face such a scenario you are speaking about they could actively design their app with that in mind (i.e. by manually reserving enough space between custom UI elements and the pager buttons of this control).

Important here: I don't have any data to back up my assumptions how common or uncommon a scenario is. The control should be designed with the most common scenarios in mind.

MarissaMatt commented 4 years ago

@chingucoding @Felix-Dev @gvazquez8 I agree that the behavior should be when the button is set to HiddenOnEdge, there will still be space reserved for it. I do agree that for some scenarios I've seen, the pager only had next and last buttons and did not have first and last buttons and I think it would be fairly common to only have next and previous buttons only.

I think @chingucoding's point is an interesting one if the developer chooses to hide and show buttons based on other criteria so I will need to think about that scenario. My initial thinking is when the visibility is set to AlwaysVisible or HiddenOnEdge, we reserve space for the button and if the visibility is set to None then we do not reserve space. That does not handle the case @chingucoding made but I am not sure how often developers would dynamically change the visibility.

marcelwgn commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

marcelwgn commented 3 years ago

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

SavoySchuler commented 3 years ago

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