Skip to content

Move collection types to MVVM Toolkit #151

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

Merged
merged 3 commits into from
Mar 19, 2022

Conversation

Sergio0694
Copy link
Member

@Sergio0694 Sergio0694 commented Mar 14, 2022

Closes #138

This PR moves the collection types with no further changes, other than the namespace.
Follow up API changes will be done in a separate PR.

@Sergio0694 Sergio0694 added feature 💡 A new feature being implemented improvements ✨ Improvements to an existing functionality introduce breaking changes 💥 This change would be a breaking change next preview ✈️ This changes will be available in the upcoming preview optimization ☄ Performance or memory usage improvements priority 🚩 An issue or change that has priority mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit labels Mar 14, 2022
@Sergio0694 Sergio0694 force-pushed the dev/collections-to-mvvm-toolkit branch from 76f36fb to bb33459 Compare March 15, 2022 19:29
@jamesmontemagno
Copy link

🎉🎉🎉🎉

@Sergio0694 Sergio0694 force-pushed the dev/collections-to-mvvm-toolkit branch from bb33459 to 68eb1fd Compare March 16, 2022 14:30
@jamesmontemagno
Copy link

Will this have things like AddRange, remove range, etc? Similar to https://github.com/jamesmontemagno/mvvm-helpers/blob/master/MvvmHelpers/ObservableRangeCollection.cs

@Sergio0694
Copy link
Member Author

Hey @jamesmontemagno, that's actually something that I specifically don't want to add. We've talked about this with @michael-hawker and @brminnick (who also had the same question, as they currently have similar helpers in the MAUI Community Toolkit, which are being removed as they standardize towards the MVVM Toolkit), and my position was that we should instead try to help make dotnet/runtime#65101 happen, and then just use that. Currently the PR is in the works, we mostly just need to talk to the WinUI 3 team to see if they can support it too (not being open source we can't just contribute to it like with WPF), and confirm that MAUI will support that too (maybe you and Brandon can help clarify this?). With that, it'll be a much better end result for consumers, as the API having proper support both has much better performance, as well as offering better UI animations (the UI can only animate the replaces items instead of resetting the whole collection). As we said before, the goal here is to put together the best possible API set for consumers in the long term, and we feel like this is the right direction to achieve that 🙂

Hope this helps!

@michael-hawker
Copy link
Member

Anything new or added here? This is just a namespace/package change, right? Curious why they're not all showing as moves.

@Sergio0694
Copy link
Member Author

@michael-hawker Yes there's a bunch of changes I want to discuss in today's API review 🙂

@michael-hawker
Copy link
Member

Capturing notes from discussion today:

  1. Separate this into two PRs, one for the file/namespace change only.
  2. New PR for the functional changes.

That'll make things easier to review. :)

For the new PR:

  • We also discussed cleaning up the interfaces to align better to the BCL and provide additional value with other existing interfaces like ILookup.
  • It also was discussed to evaluate and change the extension methods to not clash with LINQ methods and either rename or align with better docs/samples in the future.

@Sergio0694 Sergio0694 force-pushed the dev/collections-to-mvvm-toolkit branch from 68eb1fd to 5451972 Compare March 18, 2022 13:50
@Sergio0694 Sergio0694 marked this pull request as ready for review March 18, 2022 13:51
@jamesmontemagno
Copy link

Ahhhh i gotcha now, yeah that makes sense looking at the docs what this is. Still great ;)

@Sergio0694
Copy link
Member Author

YOLO-merging this one since it's just the move we discussed, so I can open the new PR with API changes 😄

@Sergio0694 Sergio0694 merged commit 460ebc9 into main Mar 19, 2022
@delete-merged-branch delete-merged-branch bot deleted the dev/collections-to-mvvm-toolkit branch March 19, 2022 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 💡 A new feature being implemented improvements ✨ Improvements to an existing functionality introduce breaking changes 💥 This change would be a breaking change mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit next preview ✈️ This changes will be available in the upcoming preview optimization ☄ Performance or memory usage improvements priority 🚩 An issue or change that has priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move grouped collections to MVVM Toolkit
3 participants