Feature/spectrogram scrolly #17
Conversation
| tabWidth: 2 | ||
| semi: true | ||
| singleQuote: true | ||
| printWidth: 150 | ||
| printWidth: 80 |
There was a problem hiding this comment.
Bonne idée, cela dépasse de l'écran autrement.
| const D3ComponentScrollyTelling = ({ | ||
| callback, | ||
| data, | ||
| isInitialized, |
There was a problem hiding this comment.
isLoaded et setIsLoaded?
There was a problem hiding this comment.
En fait, ces variables sont plutôt utilisées pour définir si la visualisation a été initialisée ou non. C'est donc utilisé plus tard quand on va appeller les callbacks (ex. spectrogramCallbacks['W']) quand on va scroller à travers un waypoint.
Le nom est isInitialized est alors, selon moi, plus appropriée, comme on ne load pas de données dans ce cas (désolé, c'est moi qui était confuse tout à l'heure en expliquant pourquoi on faisait ça).
| @@ -1,7 +1,7 @@ | |||
| import _ from 'lodash'; | |||
|
|
|||
| import { convertTimestampsToDates } from '../utils'; | |||
| import { STAGES_ORDERED, EPOCH_DURATION_SEC } from '../constants'; | |||
| import { convertTimestampsToDates, convertEpochsToAnnotations } from '../utils'; | |||
There was a problem hiding this comment.
C'est peut-être plus le rôle du back-end de convertir les epochs de 30s en plage de stade de sommeil contigües. Pour l'instant c'est ok comme ça fonctionne, mais je me dis que ce sera important de déplacer ça.
| .append('g') | ||
| .attr( | ||
| 'transform', | ||
| `translate(${MARGIN.LEFT + spectrogramWidth}, ${MARGIN.TOP})`, |
There was a problem hiding this comment.
J'imagine que ce serait mieux de faire la différence entre les margin de la légende et du spectrogramme. Peut-être créer deux objets même s'ils ont les mêmes valeurs serait préférable.
There was a problem hiding this comment.
Dans ce cas-ci, on ne définit pas le margin de la légende, mais plutôt le déplacement à faire en X pour arriver à la position voulue de la légende. On additionne donc le margin de droite, la largeur du spectrogramme, et on arrive à l'endroit en X où mettre la légende.
J'allais suggérer de faire une constante pour définir les positions en X et Y, mais comme il faudrait en fait faire une fonction qui prend en paramètre spectrogramWidth, je ne crois pas que ça l'aiderait tant à la lisibilité.
| g | ||
| .append('text') | ||
| .attr('x', spectrogramWidth / 2) | ||
| .attr('y', -MARGIN.TOP / 3) |
There was a problem hiding this comment.
Peut-être créer une constante MARGIN_TITLE = {LEFT: ..., ...., BOTTOM: ...};
There was a problem hiding this comment.
Il faudrait faire une fonction qui prend en paramètre spectrogramWidth.. je ne pense pas que ça l'aiderait à la lisibilité. Aussi, c'est plutôt les positions en x et en y, et non les margins. J'ai ajouté une variable pour la position en Y
Task link
Summary
Modifications include turning the spectrogram visualization to a scrollytelling.
It involved:
D3ComponentScrollyTellingOther modifications not related to the spectrogram (oups):
The previous printWidth was too wide for my computer screen, and, in my opinion, made the code harder to read (can be debated). I did not run the linter against all files, since it can be done au fur à mesure.