VilledeMontreal / angular-ui

https://services.montreal.ca/bao-storybook/
MIT License
5 stars 9 forks source link

fix: force refresh thumbnail and performance improvement #211

Closed mt-anna closed 3 months ago

mt-anna commented 3 months ago
mt-anna commented 3 months ago

https://github.com/user-attachments/assets/9102172a-d9d7-4861-b35d-05ae542c80b3

paul-lemsg commented 3 months ago

peut-être peux-tu ajouter un get file() pour éviter de faire ._file ?

mt-anna commented 3 months ago

@paul-lemsg non, on veut éviter les getters, le _file set aussi utilisé dans .html.

MaudeLaflamme commented 3 months ago

Pourquoi faut-il éviter les getters ? Ceux-ci peuvent être utilisés dans le template également, tu pourrais accéder à 'file' dans le .html. Peux-tu modifier le message de commit pour mettre un préfix: fix: force refresh thumbnail. Et ta PR vise présentement la branche master qui est sur Angular 16, est-ce que c'est bien la version de la librairie que ton application utilise ?

mt-anna commented 3 months ago

Les getters degradent la performance car ils sont refraichis chaque 1 sec car Angular n'a pas de moyen natif de savoir si la valeur retournée par un getter a changé sans l'appeler. J'ai remarqué que les file-input component prends visiblement de temps pour charger quand on est sur la page pour la premiere fois.

mt-anna commented 3 months ago

@MaudeLaflamme done

oui, nous sommes sur la version 16.

MaudeLaflamme commented 3 months ago

@mt-anna si le getter ne fait que retourner la valeur, il n'y a pas de raison que cela prenne du temps. Il y avait effectivement un get filesize() qui lui, appelait une méthode à chaque fois ce qui devait certainement affecter le rendement, mais pour un get file() return this._file;, je ne vois pas le ralentissement possible. À toi de voir, c'est un nitpick pour moi, je trouve simplement les getters plus élégant que d'accéder à l'attribut directement, mais je ne bloquerai pas la PR pour ça :)

MaudeLaflamme commented 3 months ago

@mt-anna remettre le fix: dans le message de commit, il est disparu !

mt-anna commented 3 months ago

@MaudeLaflamme c'est fait