-
Notifications
You must be signed in to change notification settings - Fork 25
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
refactor/ui(directions): directions component refactor #1685
base: next
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Premier tests
- À l'ajout d'un premier point , on déclanche la requête et on obtient une erreur. Tout comme au deuxième
- Modifier la coordonée directement dans l'input ne change pas la position sur la carte et ne trigger pas une nouvelle recherche d'itinéraire.
Proposition:
- Est-ce qu'on pourrait afficher les boutons de drag et de supression seulement lorsqu'on hover la ligne avec le curseur?
- Avoir au moins un point de départ et d'arrivée avant de proposer d'ajouter un point intermédiaire?
- Google Map, au lieu d'ajouter un point intermédiaire, ils utilisent ajouter une destination et il positionne le bouton à la fin.
'\n'; | ||
localCnt++; | ||
}); | ||
// URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idéalement supprimer les commentaires qui n'ajoute pas de valeur. Si c'est pour indiquer des sections d'une méthode c'est souvent un signe qu'on pourrait la décomposer en sous-méthode pour améliorer la lisibilité. Par exemple, getSummary(), getUrl(), getBody()
* | ||
* @return {string | undefined} The generated link, or undefined if the route service is not available. | ||
*/ | ||
private getLink(): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On pourrait corriger le type de retour pour string | undefined
comme en commentaire
.all() | ||
.map((stop) => roundCoordTo(stop.coordinates, 6)); | ||
let directionsUrl = ''; | ||
.map((stop: Stop) => roundCoordTo(stop.coordinates, 6)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On n'est pas obligé d'ajouter les types lorsque Typescript est capable de le déduire. Par exemple ici (stop: Stop)
, la définition du type est optionnel parce que le stopsStore indique déjà le type de la propriété view et le bon type sera déduit par Typescript. L'intellisense nous donne une bonne indiquation si l'inférence de Typescript fonctionne ou non et parfois on doit l'ajouter mais dans la majorité du cas ça l'ajoute une pollution visuel.
Tout comme let directionsUrl: string = '';
vu qu'on initialise la variable avec un string vide, Typescript peut déjà déduire le bon type.
<div class="igo-input-container" *ngIf="directions && activeDirection"> | ||
<mat-form-field *ngIf="directions && directions.length > 1"> | ||
<div class="igo-input-container" *ngIf="routes && activeRoute"> | ||
<mat-form-field *ngIf="routes?.length > 1" [matTooltip]="activeRoute.title"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On peut enlever le optionnal chaining on a une condition qui vérifie si routes est défini
formatDuration(activeRoute.duration) + | ||
')' | ||
: '' | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On pourrait faire une méthode getRouteTitle
pour améliorer la lisibilité du template
@@ -64,7 +68,7 @@ export interface SourceProposal { | |||
meta: SearchMeta; | |||
} | |||
|
|||
export interface Direction { | |||
export interface Directions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pourquoi le pluriel? Habituellement, c'est utilisé pour représenter une Liste
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Purement sémantique, en anglais 'Direction' est une direction cardinale (nord, est, droite, gauche), 'Directions' est la série d'instructions pour aller à un endroit, ce qui fait plus de sens dans le contexte. Google Maps parle de 'Directions' par exemple
@@ -131,146 +137,191 @@ export class DirectionsService { | |||
doc.setFontSize(10); | |||
doc.text(attributionText, xPosition, yPosition); | |||
} | |||
} | |||
}*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Faire le ménage, supprimer le code commenté
@@ -27,6 +27,7 @@ import { | |||
PrintPaperFormat, | |||
PrintResolution | |||
} from './print.type'; | |||
import { Size } from 'ol/size'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On ajoute une nouvelle dépendance à Openlayers. Idéalement, on aimerait faire abstraction de Ol pour les types lorsqu'on peut le faire.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je vais changer pour number[]
@Input() debounce: number = 200; | ||
@Input() length: number = 2; | ||
@Input() coordRoundedDecimals: number = 6; | ||
@Input() zoomToActiveRoute$: Subject<void> = new Subject(); | ||
@Input() zoomOnActiveRoute$: Subject<void> = new Subject(); | ||
@Input() authenticated$: BehaviorSubject<boolean>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On pourrait améliorer la composition en spécifiant quel @input est requis, par ex: @Input({ required: true })
Please check if the PR fulfills these requirements
What is the current behavior? (You can also link to an open issue here)
After having deployed the private routing option and having multiple feedback from different users on the component, many features were not clear or unknown to many of them and/or the UI was not user-friendly.
What is the new behavior?
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications:
Other information: