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

SampleApp updates and more #1624

Merged
merged 35 commits into from
Nov 16, 2017
Merged

SampleApp updates and more #1624

merged 35 commits into from
Nov 16, 2017

Conversation

nmetulev
Copy link
Contributor

Issue: #1539 #1592

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[x] Code style update (formatting)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build or CI related changes
[ ] Documentation content changes
[x] Sample app changes
[ ] Other... Please describe:

PR Checklist

Please check if your PR fulfills the following requirements:

What is the new behavior?

This PR introduces the following changes:

  • Updated Sample App with more fluent design touches such as animations and materials
  • Replaced icons with new and improved icons
  • Fixed sample app to always grab latest documentation from new docs repo (Discrepancy between the app and NuGet package. #1592)
  • Fixed formatting warnings in various packages
  • Fixed a bug with implicit animations crashing on 14393
  • Fixed minor bugs with sample app

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

nmetulev and others added 30 commits November 4, 2017 18:57
Add Focus on Xaml Pivot Item Selection
Add Tracking for Internal Monaco Editor Exceptions
Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Approving to move things forward, couple of comments, but nothing critical. Though did you test on XBox?

@@ -145,7 +145,7 @@
</ItemGroup>
<ItemGroup>
<!-- A reference to the entire .Net Framework and Windows SDK are automatically included -->
<Content Include="Assets\Helpers.png" />
<Content Include="Assets\Helpers_old.png" />
Copy link
Member

Choose a reason for hiding this comment

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

What's old about this now? ;)

Can it be removed?

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, will removed

<Extension Category="windows.backgroundTasks" EntryPoint="Microsoft.Toolkit.Uwp.Samples.BackgroundTasks.TestBackgroundTask">
<BackgroundTasks>
<Task Type="timer" />
</BackgroundTasks>
</Extension>
<uap:Extension Category="windows.protocol">
Copy link
Member

Choose a reason for hiding this comment

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

Reason this moved down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It must have moved down when the I've created an appx for testing.


namespace Microsoft.Toolkit.Uwp.SampleApp
{
public static class XAMLHelper
Copy link
Member

Choose a reason for hiding this comment

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

Should this be AcrylicHelper? Would help in readability when used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had it named AcrylicHelper originaly, but I wanted to also add helpers for Reveal later on so renamed it to be more generic

Copy link
Contributor

Choose a reason for hiding this comment

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

Name should be XamlHelper


public async Task StartSearch(string startingText = null)
{
if (_searchBox == null || _searchBox.Visibility == Visibility.Visible)
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be actually nice if you hit 'enter' in the blank searchbox that it shows all the controls (it does this if you type something and then hit backspace).

Did you get Shane to test these changes on the XBox?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, fixed. Tested it on Xbox too

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just show everything as soon as you click the search button and then filter down from there

Copy link
Contributor

Choose a reason for hiding this comment

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

Having the animation when filtering is very jarring. This should be disabled

@skendrot
Copy link
Contributor

When using the app with a narrow width, the menu button is confusing. I don't expect the items to be scrollable. I would expect the items to be in a standard menu when clicking the button

@skendrot
Copy link
Contributor

skendrot commented Nov 15, 2017

Found a bug when in narrow mode.

  1. Open Menu
  2. Click Controls (might have to click twice to get the menu to go away)

Notice the menu button is still rotated 90 degrees.

@nmetulev
Copy link
Contributor Author

@skendrot, fixed the menu bug, good catch. Narrow view still needs improvement, will probably get that done for 2.2

@nmetulev nmetulev merged commit ae3771b into master Nov 16, 2017
@nmetulev nmetulev deleted the nmetulev/sampleapp branch November 16, 2017 01:26
@windowstoolkitbot
Copy link

This PR is linked to unclosed issues. Please check if one of these issues should be closed: #1539, #1592

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.

5 participants