Skip to content

Add CI mode button#2881

Merged
StephenLPeters merged 5 commits intomicrosoft:masterfrom
marcelwgn:testapp-cidimensions
Aug 3, 2020
Merged

Add CI mode button#2881
StephenLPeters merged 5 commits intomicrosoft:masterfrom
marcelwgn:testapp-cidimensions

Conversation

@marcelwgn
Copy link
Collaborator

Description

Adds a button which sets the inner pages dimensions to that of the CI.

Motivation and Context

Fixes #2604

How Has This Been Tested?

Tested manually

Screenshots (if appropriate):

image

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Jul 10, 2020
@ranjeshj ranjeshj added area-DevInternal Internal build infrastructure, code cleanup, engineering efficiency team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Jul 10, 2020

private void _innerFrameInLabDimensions_Click(object sender, RoutedEventArgs e)
{
if(Math.Abs(_pagePresenter.MaxWidth - 1024) < 5)
Copy link
Contributor

Choose a reason for hiding this comment

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

if(Math.Abs(_pagePresenter.MaxWidth - 1024) < 5) [](start = 10, length = 50)

Why the tolerance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No particular reason, do you want me to use a smaller number than 5 for the tolerance?

Copy link
Contributor

Choose a reason for hiding this comment

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

if there not needed can we just check for equality?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm a bit hesitant to directly compare them as they are floats at the end of the day, but if you say that it won't really matter here, sure I'll use a direct comparison.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, especially if this value gets dpi scaled, but i think we should be deliberate about the tolerance we choose, scrollview has an epsilon value that I think would be a good starting spot. @gegao18 do you know if this value gets scaled by the system, if so would you suggest scaling our epsilon?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated the check. It is now checking for the default value of the presenter MaxWidth, which works since we are resetting the value. That way, we avoid potential issues with precision and DPI scaling.

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@marcelwgn
Copy link
Collaborator Author

Fixed the test failures for the repeater API test and the TabView interaction test. I am not sure why the three radiobuttons interaction tests failed, they only failed on RS3 and RS4 and work on my machine.

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@marcelwgn
Copy link
Collaborator Author

Seems that the RadioButton tests fail consistently, but only on RS3 and RS4. Could it be that focus/tab behavior is different on those versions?

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

if(double.IsInfinity(_pagePresenter.MaxWidth))
{
// Not CI mode, so enter it now
_pagePresenter.MaxWidth = 1024;
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests don't actually use this button right? Also, I'm trying to figure out why the tests are failing in the lab and I"m noticing that my local run includes I think 1 extra pixel than the screenshot from the lab shows. Is this number possibly off by one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes the tests don't actually use that button, it is only designer for developers to check if there are possible problems with the test page size. It could be that the number is of by one, measuring the screenshots is not the most precise method.

Could you tell me what number of pixels it should be instead?

@StephenLPeters
Copy link
Contributor

I think I figured out the issue, on RS3 and RS4 the page lays out slightly differently which causes the Focus Selected Item button to be a few pixels lower than on RS5. Your change also pushed the UI down a few pixels, the combination results in the center of the Focus Selected Item Button being under the horizontal scroll bar at the bottom of the page. Adjusting the UI a bit or scrolling the button into view should solve the issue.

@StephenLPeters
Copy link
Contributor

@chingucoding pinging about the updates

@marcelwgn
Copy link
Collaborator Author

I've changed the CI modes button to be smaller, maybe this fixes the issue. I am a bit confused why the button caused this issue in the first place. Did it change the height of the top padding? I thought it would have the same height as the other buttons on the top area.

Thanks for the ping btw.

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@StephenLPeters StephenLPeters left a comment

Choose a reason for hiding this comment

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

:shipit:

@StephenLPeters
Copy link
Contributor

I've changed the CI modes button to be smaller, maybe this fixes the issue. I am a bit confused why the button caused this issue in the first place. Did it change the height of the top padding? I thought it would have the same height as the other buttons on the top area.

Thanks for the ping btw.

I suspect that ToggleButton has a slightly larger footprint than button? Looking at their templates I'm not sure that is the case but It's the only source I can think of

@StephenLPeters StephenLPeters merged commit 93d6555 into microsoft:master Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-DevInternal Internal build infrastructure, code cleanup, engineering efficiency team-Controls Issue for the Controls team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add button to test app that set's inner frame size to CI dimensions

4 participants