ajout de la version WebExtensions pour chrome#46
Conversation
There was a problem hiding this comment.
J’ai juste regardé les sources, je n’ai pas essayé plus que ça.
Du plus important au moins important :
- J’aime bien le fait que ce soit beaucoup plus simple qu’actuellement
- J’aime bien le fait que ça utilise l’API comme il faut sans scrap le HTML
innerHTMLc’est mal- Ça serait plus jolis en découpant la grosse fonction
getNotificationsFromAPI() - J’aime moins le fait que ça utilise des XHR au lieu de
fetch() - J’aime moins le fait qu’il n’y ai pas trop de réglages (on ne peut visiblement pas changer l’intervalle de rafraichissement et tout)
Google Chrome/notifier.js
Outdated
| divBlocNotif.id="blocNotif"; | ||
| var divDate = document.createElement('div'); | ||
| divDate.id = "date"; | ||
| divDate.innerHTML = escapeHTML(date); |
There was a problem hiding this comment.
Pourquoi ne pas utiliser innerText ? Il n’y aurait pas besoin de escapeHTML().
There was a problem hiding this comment.
Pour aucune raison valable...
Google Chrome/notifier.js
Outdated
| function getNotificationsFromAPI() { | ||
| _contentDiv = document.createElement('div'); | ||
| var target = _base_url + "api/notifications/?page_size=30&ordering=-pubdate&Authorization=" + _token; | ||
| var xhr = new XMLHttpRequest(); |
There was a problem hiding this comment.
Tout le tralala sur les XHR devrait pouvoir être retiré parce que maintenant, en ces temps modernes, il y a fetch(). Mais bon, ça peut attendre.
There was a problem hiding this comment.
Effectivement, dans un environnement contrôlé comme celui des extensions on peut se permettre. Ça devrait pas faire de mal pour nettoyer le code de tout passer en Promise (voire en Observable quand ce sera natif)
There was a problem hiding this comment.
Je vois pas trop l’intérêt des observables dans ce cas. Pour moi, c’est surtout utile pour des flux d’évenements, là on récupère juste une seule page web. Puis les promises marchent avec async et await.
There was a problem hiding this comment.
La liste des notifs pourrait être un observable (à terme, hein, dans l'immédiat ça sert à rien) avec une récupération en temps réel, en fait
Google Chrome/notifier.js
Outdated
| //Get new notifications | ||
| var resultsNotification = rootDOM.results; | ||
| var countNotifications = 0; | ||
| for(var notif = 0; notif < resultsNotification.length; ++notif) { |
There was a problem hiding this comment.
Bon ça serait cool de déplacer ça dans une fonction parce qu’il y a des if dans des for dans des if dans des if dans des if…
There was a problem hiding this comment.
👍
À noter qu'un for (const notif of results) serait pas mal pour nettoyer le code également.
Google Chrome/notifier.js
Outdated
| /* | ||
| * Global variables | ||
| */ | ||
| var _notifCounter = 0; |
There was a problem hiding this comment.
Ça serait mieux de mettre ces variables globales dans une IIFE ou un module de ES6 plutôt que de les préfixer par des _.
There was a problem hiding this comment.
On pourrait d'ailleurs utiliser la nouvelle notation à base de const et let pour ce genre de choses…
Google Chrome/notifier.js
Outdated
| //If we are in debug mode | ||
| var _debug = false; | ||
| var _base_url = "https://zestedesavoir.com/"; | ||
| var _token = "zds-notifier-firefox"; |
Google Chrome/notifier.js
Outdated
| if(_debug) _base_url = "https://beta.zestedesavoir.com/"; | ||
|
|
||
| function escapeHTML(str) { return str.replace(/[&"'<>]/g, (m) => escapeHTML.replacements[m]); } | ||
| escapeHTML.replacements = { "&": "&", '"': """, "'": "'", "<": "<", ">": ">" }; |
There was a problem hiding this comment.
À mon humble avis, c’est inutile si tu utilises innerText.
There was a problem hiding this comment.
Yeap. J'ai relu le code et il était bien bien moche :D
|
J'ai pas encore fait le tour du code en détail, mais tes remarques sont justes, ça fait déjà une base de choses à corriger. Il faudrait aussi passer à ES6 de façon générale, autant que possible (voir caniuse si besoin pour la compatibilité), pour garder un code simple, moderne et maintenable. |
|
Je tente de faire ça ce week end ici ! |
|
Bon j'ai pas eu le temps (prend plus de temps que prévu) de tout faire ce week end mais j'ai pas oublié et c'est en cours :) |
AmarOk1412
force-pushed
the
chrome-webext
branch
from
f74e393 to
b887a00
Compare
|
Bon, on va déterrer un peu tout ça ! Je viens d'installer l'extension chez moi et j'ai déjà un bug : ça se rafraîchit pas. Du coup j'ai beau avoir cliqué sur un lien, lu le contenu et attendu un petit moment, j'ai toujours la notif. Aussi les notifs sont de la même couleur que le header/footer de la popup, c'est pas super visible. Ce serait pas mieux avec un fond clair pour la liste ? Ensuite la page de settings est pas hyper sexy (le |
3 participants
Hei @viki53, @Eskimon, j'ai enfin pris le temps de faire les quelques modifs pour la version webextension.
QA
Note
Le code est exactement le même que la version Firefox. Ce qui change :
notifier_popup.css(j'ai fais un truc crade d'ailleurs dedans)Du coup je me dis qu'il est plus judicieux de mettre tout dans le même dossier et séparer juste le css et le manifest en précisant dans le README la nouvelle démarche.
Qu'en pensez-vous ?