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

Added support for binding to sub-objects in ComboBoxColumn #2919

Merged
merged 19 commits into from May 28, 2020

Conversation

filipwa84
Copy link
Contributor

@filipwa84 filipwa84 commented May 2, 2019

Issue: #2738

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

Binding cannot be bound to sub-objects and throws an exception.

What is the new behavior?

Support for binding to sub-objects has been added.

PR Checklist

Please check if your PR fulfills the following requirements:

  • [x ] Tested code with current supported SDKs
  • [x ] Pull Request has been submitted to the documentation repository instructions. Link:
  • [x ] 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)
  • [x ] Contains NO breaking changes

Other information

@filipwa84 filipwa84 changed the title Features/sub objects Added support for binding to sub-objects in ComboBoxColumn May 2, 2019
@filipwa84
Copy link
Contributor Author

@RBrid Would you be able to review this pull request for me?

@michael-hawker michael-hawker added this to the 6.0 milestone Jul 17, 2019
@michael-hawker michael-hawker added reviewers wanted DataGrid 🔠 Issues on DataGrid control labels Aug 29, 2019
@filipwa84
Copy link
Contributor Author

@RBrid Hi Regis, would you have time to look at this one please?

Copy link
Contributor

@RBrid RBrid left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay Filip. There is already a file with a bunch of type utility functions: ...\WindowsCommunityToolkit\Microsoft.Toolkit.Uwp.UI.Controls.DataGrid\Utilities\TypeHelper.cs

  • Can you please check if any of those methods can be used instead of yours?
  • Any new method should be added to that file (if any is still required) instead of DataGridComboBoxColumn.cs. And if you add some to that file it's better to make them bullet proof with input parameter checks.

Thanks!

@filipwa84
Copy link
Contributor Author

@rbird Thanks for the comments. Will look in to it next week and replace with methods from TypeHelper.cs

@michael-hawker michael-hawker modified the milestones: 6.0, 6.1 Oct 24, 2019
Copy link
Contributor

@RBrid RBrid left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this Filip.

Copy link
Contributor

@RBrid RBrid left a comment

Choose a reason for hiding this comment

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

Thanks for the changes Filip.

@filipwa84
Copy link
Contributor Author

@michael-hawker Any chance of getting another reviewer to approve this pull request? :)

@michael-hawker
Copy link
Member

@filipwa84 sorry this fell off my radar with the holidays. Régis' approval is the main gate, looks good to me.

Is there anything in the sample page that should be updated to show off this new feature?

@filipwa84
Copy link
Contributor Author

@michael-hawker Nothing that needs to be shown in the sample app in my opinion. The added functionality just adds the possibility to bind to sub-objects which is an expected behavior.

@Kyaa-dost Kyaa-dost added this to In progress in Bugs 6.1 via automation Feb 18, 2020
@michael-hawker michael-hawker moved this from In progress to Review in progress in Bugs 6.1 Apr 2, 2020
@michael-hawker michael-hawker moved this from Review in progress to Review approved in Bugs 6.1 Apr 2, 2020
@michael-hawker
Copy link
Member

@filipwa84 sorry for the delay on this, any advice on framing for testing this feature?

Also, it appears there's a merge conflict now, would you mind resolving?

Thanks!

@michael-hawker michael-hawker moved this from Review approved to Review in progress in Bugs 6.1 Apr 2, 2020
@filipwa84
Copy link
Contributor Author

@michael-hawker I'll take a look at it

@filipwa84
Copy link
Contributor Author

filipwa84 commented May 6, 2020

@michael-hawker The merge conflict is resolved.

Regarding testing, you can just verify that the column's Binding property can be bound to a property within a sub-object. For example:

<controls:DataGrid AutoGenerateColumns="False"
			        ItemsSource="{x:Bind MyModels, Mode=OneWay}"
				SelectedItem="{x:Bind SelectedMyModel, Mode=TwoWay}">
	<controls:DataGrid.Columns>
		<controls:DataGridComboBoxColumn Binding="{Binding MySubModel.DisplayId}" ItemsSource="{x:Bind Displays}" DisplayMemberPath="Text"/>
	</controls:DataGrid.Columns>

</controls:DataGrid>

where MySubModel is a "sub-object" inside MyModels which has a property called DisplayId which we are matching to a property with the same name inside Displays.

@filipwa84
Copy link
Contributor Author

@michael-hawker any chance of getting this merged? :) We rely on this being fixed in our project...

@michael-hawker
Copy link
Member

@filipwa84 yup, I'll get this merged for 6.1, just haven't had time to test things out, thanks for the example. I'll use that and take a look, thanks a bunch!

Bugs 6.1 automation moved this from Review in progress to Review approved May 28, 2020
@michael-hawker
Copy link
Member

Thanks for the contribution @filipwa84! Sorry this took so long to merge and missed the last release. It'll be 6.1 though! 🎉🎉🎉

@michael-hawker michael-hawker merged commit 26d00f2 into CommunityToolkit:master May 28, 2020
Bugs 6.1 automation moved this from Review approved to Done May 28, 2020
@filipwa84
Copy link
Contributor Author

@michael-hawker Thank you. Just a quick question, should this be available in the NuGet preview package now? Tried to install 6.1 preview but it seemed like it wasn't there...

@michael-hawker
Copy link
Member

@filipwa84 I think this was merged after we did the preview package for //Build. You can grab it from MyGet, those are updated as soon as it gets merged to Master automatically.

6.1 should ship on Monday though! 🤞

Thanks again for this contribution! 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DataGrid 🔠 Issues on DataGrid control improvements ✨
Projects
No open projects
Bugs 6.1
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants