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

OnDevice markup extension added #2664

Closed
wants to merge 12 commits into from
Closed

OnDevice markup extension added #2664

wants to merge 12 commits into from

Conversation

sonnemaf
Copy link
Contributor

Issue: #2489

PR Type

What kind of change does this PR introduce?

Feature

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

@michael-hawker
Copy link
Member

Can you split out the .editorconfig changes? These two PRs from you seem intertwined. It'd be nice maybe if you could start fresh for each with a branch from master with just one commit with the new files?

}
else
{
deviceFamily = Windows.System.Profile.AnalyticsInfo.VersionInfo.DeviceFamily;
Copy link
Member

Choose a reason for hiding this comment

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

Use our AnalyticsInfo extension method instead of duplicating this logic here with strings, then you can use the enum returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The AnalyticsInfo extension also supports a Tablet value. Should OnDevice have a Tablet property?

Why is a tablet different than a Desktop, only the Touch versus Mouse. What if I connect a mouse to my tablet. I prefer to keep it as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The AnaliticsInfo extension is part of the SampleApp and must be moved to Microsoft.Toolkit befor it can be used. Do we do this?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I forgot it was only part of the sample, I think it'd make sense to move as an Extension in the Microsoft.Toolkit.Uwp namespace.

The tablet mode is a specific mode you can put windows in:
image
I think it'd be useful to have Tablet as an option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I will do that. Thanks

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 have second thoughts about a Tablet property. See my code below. The Text of The Textblock would be Hello when running on Desktop and World when running on Tablet, otherwise it is Hi (Xbox, Holo, IoT, etc). What happens when the user is running the app and then switches from Desktop to Tablet mode. Would the Text have to change on the fly? If yes, how do we implement this?

<TextBlock Text="{helpers:OnDevice Default=Hi, Desktop=Hello, Tablet=World}"
           xmlns:helpers="using:Microsoft.Toolkit.Uwp.UI.Extensions.Markup" />

Copy link
Contributor

@nmetulev nmetulev left a comment

Choose a reason for hiding this comment

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

Make sure to remove the unnecessary files in this PR and only keep the files that are relevant

@sonnemaf
Copy link
Contributor Author

@nmetulev I understand that the .editorconfig has to be removed. How do I do that? I'm a daily TFS user and not very experienced in GitHub. Can you help me?

@michael-hawker
Copy link
Member

@sonnemaf It'd probably be easiest at this point to checkout a fresh master, create a new branch off of that and copy over your OnDevice file, create the new extension in the Microsoft.Toolkit.Uwp namespace, and update your code and the sample app to use it.

@sonnemaf
Copy link
Contributor Author

@michael-hawker I will do that. Thanks.

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.

4 participants