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

Experimental attribute for GraphPresenter and sample updates #148

Merged
merged 3 commits into from
Aug 31, 2021

Conversation

shweaver-MSFT
Copy link
Member

Fixes #55

PR Type

What kind of change does this PR introduce?

  • Sample app changes

What is the current behavior?

We are getting close to release, and the GraphPresenter is not quite completed. There are a few additional features to add and scenarios to test before it will be ready for regular use.

Also, the sample app has several GraphPresenter samples, but they are all sharing the same file. This makes it a bit harder to read the code.

What is the new behavior?

I've added the Experimental attribute to the GraphPresenter and associated QueryOption classes. This warns developers who choose to consume them.

Next, I've taken the existing GraphPresenter sub-samples and moved them into individual pages in their own folder. This cleans up the sample a bit and makes it easier to see each sample in isolation and understand how it works.

Lastly, I've added an additional GraphPresenter sub-sample for OneDrive as detailed in #55.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • 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

@ghost
Copy link

ghost commented Aug 26, 2021

Thanks shweaver-MSFT for opening a Pull Request! The reviewers will test the PR and highlight if there is any merge conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@shweaver-MSFT
Copy link
Member Author

@michael-hawker added you in case you are curious. This was based on your sample in #55. It works great! Pretty easy to drop in and make a few adjustments 😊

</Grid.ColumnDefinitions>
<controls:ImageEx
Grid.RowSpan="2"
DataContext="{x:Bind RemoteItem, Converter={StaticResource OneDriveThumbnailConverter}}"
Copy link
Member

Choose a reason for hiding this comment

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

I suppose since we're just doing a sample and using x:Bind here already it'd be easier to just use a function call vs. a converter, eh? (Would probably remove half the lines in the code-behind for us... 😋

I think I originally used a converter as I intended that converter may end-up in the Toolkit or a control template in the long run, but that's something we can re-adapt later if needed vs. having a simpler to use function in the code behind for a sample.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point 😅 I didn't even think about that. I'll make them methods for simplicity/consistency.

@michael-hawker
Copy link
Member

Yeah, I'll pull things down tomorrow and give it a spin. Excited to check out all the sample updates. 🎉

@michael-hawker
Copy link
Member

Ah, we're not running the unit tests in the CI, eh?

Looks like we forgot to update some tests with namespace changes?

image

@michael-hawker
Copy link
Member

michael-hawker commented Aug 27, 2021

Hmm, got an "Exception = {"The operation was canceled."}" when trying to run the sample.

Edit: Cleaned and rebuilt seemed to work.

@michael-hawker
Copy link
Member

When switching to the PeoplePicker page:

image

@michael-hawker
Copy link
Member

Try testing out some of the other samples, got a scope exception and seeing some index exceptions eaten internally somewhere. But otherwise looking great! 🎉

@shweaver-MSFT
Copy link
Member Author

@michael-hawker weird exception, I haven't seen that myself. I'll see if I can play around and get it to repro.

I'll also get those tests updated 👍

@shweaver-MSFT
Copy link
Member Author

I'm not sure about the exception, so I've logged bug #149

I have a hunch that the issue is in TokeninzingTextBox and not in the PeoplePicker itself.

@shweaver-MSFT shweaver-MSFT merged commit 3ca6264 into main Aug 31, 2021
@shweaver-MSFT shweaver-MSFT deleted the shweaver/experimental branch August 31, 2021 16:18
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.

[Sample] Show using OneDrive Recent Files in GraphPresenter
2 participants