search

uwp-squad / livecoding-uwp

A non official Windows 10 client for livecoding.tv
https://www.microsoft.com/store/apps/9mzxhd72lwql
2 stars 2 forks source link

Intégration de MVVMLight #11

Closed aristid-lavri closed 7 years ago

aristid-lavri commented 7 years ago

juste un aperçu de ce que j'ai fait en attendant

Odonno commented 7 years ago

Ok, voici la liste des choses que l'on pourrait améliorer :

aristid-lavri commented 7 years ago

ok pas de soucis , si tas d'autres remarques

Odonno commented 7 years ago

J'ai redéfini le scope de la PR autour de l'intégration de MVVM.

Odonno commented 7 years ago

@aristid-lavri Le gif et l'ancienne image sont toujours présentes dans la solution. Tu peux les supprimer.

Odonno commented 7 years ago

C'est déjà mieux. En ce qui concerne les évolutions du code, j'ai quelques questions :

aristid-lavri commented 7 years ago

Le paterne repository n'est pas utilisé qu'avec des base de données dans notre cas bien vrai qu'on utilise une api mais le fait d'avoir un repository donne une surcouche d'abstraction et on ne manipulera que les données dans les vues ( on na pas besoin d'appeler l'api dans la vu) ainsi on respecte le MVVM

Odonno commented 7 years ago

Ok, je comprends ton raisonnement. Avoir une couche d'abstraction est parfois bénéfique, surtout dans des applications ayant des dépendances fortes avec des API ou des bases de données, effectivement.

Dans notre cas, étant donné que nous construisons un client léger, notre objectif est de consommer tout simplement les données sur chaque page et donc sur chaque ViewModel. Concrètement, sur la page LiveStreams par exemple, tu charges une collection de LiveStream à partir de l'API et rien de plus.

Voilà un exemple plus parlant : 

// Côté ViewModel
public ObservableCollection<LiveStream> LiveStreams { get; private set: }

// Tu mets à jour la collection dans le ViewModel à ta guise et la vue est mise à jour en temps réel, sans effort

// Côté Vue
ItemsSource="{Binding Path=LiveStreams}"

En clair, c'est ton ViewModel directement qui fait appel à l'API et c'est la vue qui consomme les propriétés et les méthodes du ViewModel. Une couche d'abstraction n'est pas nécessaire, du moins pour le moment.

aristid-lavri commented 7 years ago

ok je vois mais c'est bon d'être prévoyant :) mais c'est ok

Odonno commented 7 years ago

Certes. Mais anticiper un trop grand nombre de fonctionnalités résulte en un grand nombre de gaspillage. Le scope de la PR est sur MVVMLight, je pense qu'elle va faire plus que cela, ce qui est dommageable.

Dans tous les cas, ce qui est fait est fait. Il ne reste plus qu'à corriger certaines choses et ce sera parfait, j'en suis sûr.

Odonno commented 7 years ago

Je sais que cela fait un moment que la PR a été démarrée. J'aimerais savoir si on la continue ou s'il serait préférable de l'arrêter et d'en commencer une autre sachant que l'ajout de MVVMLight est censé être indolore. Qu'en penses-tu ?

aristid-lavri commented 7 years ago

Ca serait bien de reprendre comme ca @andre726 comprendra mieux et il pourra avancer en même temps que nous

andre726 commented 7 years ago

j'avoue que je ne sais pas faire de mvvmlight, je ne connais pas encore tout ces principe, vous penser a quoi comme pr?

aristid-lavri commented 7 years ago

je pense qu'il s'agit de reprendre celle-ci

Odonno commented 7 years ago

Dans ce cas, est-ce que ça vous dérange si je fais des modifications sur cette branche ? Concernant MVVMLight bien sûr.

aristid-lavri commented 7 years ago

pas du tout

andre726 commented 7 years ago

aucun souci

Odonno commented 7 years ago

Voilà, j'ai fait les changements. J'en ai profité pour revenir sur un modèle simple avec 2 pages : LoginPage et MainPage. Je suppose qu'on va partir avec un menu hamburger donc la page contenant ce contrôle sera sans doute la MainPage.

J'ai également rajouté un NavigationService propre à MVVMLight.

Odonno commented 7 years ago

Dites-moi ce que vous en pensez, histoire que je puisse merger la PR.