feat(*): Add a temporary disable follow position state #1636
Conversation
* Added status to the gps button. Re-center the view. Alternate color for the tracking and follow position. * Initialize temporaryDisableFollowPosition in the constructor. * Added tooltip changes and changed gps geolocate button color.
| new BehaviorSubject(5); | ||
|
|
||
| public readonly viewUpdatePositionDebounceTime$: BehaviorSubject<number> = | ||
| new BehaviorSubject(5000); |
There was a problem hiding this comment.
On pourrait utiliser seulement une valeur numérique au lieu d'un BehaviorSubject
There was a problem hiding this comment.
Car la valeur peut etre changé au besoin de l'extérieur et pour re-triggerer le subscribe basé sur ce nouveau délai.
| } | ||
| private _temporaryDisableFollowPosition: boolean; | ||
| public temporaryDisableFollowPosition$: BehaviorSubject<boolean> = | ||
| new BehaviorSubject(false); |
There was a problem hiding this comment.
J'ai l'impression qu'on a dupliquer l'état de followPosition pour avoir son opposé. J'ai personnellement une préférence pour ne pas dupliquer et conserver l'affirmatif dans nos états si possible. En plus, présentement on peut configurer followPosition mais on l'override via le constructeur en forcant temporaryDisableFollowPosition à être false
There was a problem hiding this comment.
Ce n'est pas une duplication pour avoir son opposé, c'est pour ajouter un nouvel état de suivi de position. Le temporaryDisableFollowPosition est un état différent du followPosition.
| constructor( | ||
| private map: MapBase, | ||
| private options?: MapGeolocationControllerOptions, | ||
| private storageService?: StorageService, | ||
| private configService?: ConfigService | ||
| ) { | ||
| super(); | ||
| this.temporaryDisableFollowPosition = false; |
There was a problem hiding this comment.
Cette valeur est déjà à False par défaut, pourquoi la remettre à false?
There was a problem hiding this comment.
L'observable est False par défaut, mais la valeur du Boolean temporaryDisableFollowPosition n'est pas initialisée.
| const different = this.positionsAreDifferent( | ||
| position, | ||
| this.position$.getValue() | ||
| ); |
There was a problem hiding this comment.
Dans quel cas de figure, la position serait identique? En théorie, si on se fit a this.geolocation.on('change' on ne devrait pas avoir la même position?
| @@ -501,6 +568,9 @@ export class MapGeolocationController extends MapController { | |||
| position: MapGeolocationState, | |||
| zoomTo: boolean = false | |||
| ) { | |||
| if (!position || !position.position) { | |||
There was a problem hiding this comment.
position est requis pour cette méthode, il n'y a aucune raison qu'elle soit undefined sinon c'est un bogue ou une mauvaise définition de l'interface. Même chose pour position.position?
There was a problem hiding this comment.
je reste confortable avec cette définition. test le et on s'en reparle.
| @@ -13,6 +13,7 @@ import { IgoMap } from '../shared/map'; | |||
| }) | |||
| export class GeolocateButtonComponent implements AfterContentInit, OnDestroy { | |||
| private tracking$$: Subscription; | |||
| private isTemporaryDisableFollowPositionToSwitch: Boolean; | |||
There was a problem hiding this comment.
Boolean vs boolean, on devrait toujours utiliser le dernier qui est le type primitif
There was a problem hiding this comment.
C'est corrigé
| this.map.geolocationController.temporaryDisableFollowPosition$.subscribe( | ||
| (r) => { | ||
| if (r) { | ||
| this.color = 'accent'; | ||
| } | ||
| } | ||
| ); | ||
| this.map.geolocationController.followPosition$.subscribe((follow) => { | ||
| if (follow) { | ||
| this.color = 'primary'; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Comme mentionné dans le fichier geolocation temporaryDisableFollowPosition et followPosition signifient la même chose.
| this.map.geolocationController.temporaryDisableFollowPosition$.subscribe( | |
| (r) => { | |
| if (r) { | |
| this.color = 'accent'; | |
| } | |
| } | |
| ); | |
| this.map.geolocationController.followPosition$.subscribe((follow) => { | |
| if (follow) { | |
| this.color = 'primary'; | |
| } | |
| }); | |
| this.map.geolocationController.followPosition$.subscribe((follow) => { | |
| this.color = follow ? 'primary' : 'accent'; | |
| }); |
There was a problem hiding this comment.
Les deux variables ne signifient pas la même chose. Comme mentionné dans geolocation.
| @@ -154,6 +154,9 @@ export class DirectionsComponent implements OnInit, OnDestroy { | |||
| if (activeRoute) { | |||
| activeRoute.ol.getGeometry(); | |||
| const routeExtent = activeRoute.ol.getGeometry().getExtent(); | |||
|
|
|||
| this.routesFeatureStore.layer.map.geolocationController.temporaryDisableFollowPosition = | |||
| true; | |||
There was a problem hiding this comment.
Est-ce qu'on pourrait avoir une meilleur abstraction pour éviter de devoir modifier cet état dans plusieurs fichiers. Je ne crois pas que ce soit maintenable parce que chaque fois qu'un développeut fera appel a une animation il va devoir être au courant de ce comportement.
Tandis que si on l'implémente au niveau des classe Map/View, on pourrait avoir une méthode baseAnimation qui s'occuperait d'indiquer qu'une animation a été déclancher et la classe Map pourrait réagir a des événements et mettre à off le followPosition du Geolocation.
There was a problem hiding this comment.
ca devra etre fait dans une autre pr. Actuellement, avec les test effectués, on a pas trouvé de chemin possible.
There was a problem hiding this comment.
Lorsque tu parles d'une autre PR, c'est un TODO ou elle est déjà créé? Sinon quel sont les critères pour désactiver le "follow position"? On pourrait faire des propositions pour corriger le problème.
There was a problem hiding this comment.
Non. Rien n'a été fait. Nous testons encore le code de ce pr.
| @@ -23,6 +23,8 @@ export interface MapViewControllerOptions { | |||
| * Controller to handle map view interactions | |||
| */ | |||
| export class MapViewController extends MapController { | |||
| readonly dragging$: Subject<void> = new Subject(); | |||
There was a problem hiding this comment.
Est-ce que c'est utilisé?
There was a problem hiding this comment.
Oui. Dans geolocation, dans la fonction setUpObservers. On subscribe et ça permet de changer la valeur du temporaryDisableFollowPosition
| @@ -154,6 +154,9 @@ export class DirectionsComponent implements OnInit, OnDestroy { | |||
| if (activeRoute) { | |||
| activeRoute.ol.getGeometry(); | |||
| const routeExtent = activeRoute.ol.getGeometry().getExtent(); | |||
|
|
|||
| this.routesFeatureStore.layer.map.geolocationController.temporaryDisableFollowPosition = | |||
| true; | |||
There was a problem hiding this comment.
Lorsque tu parles d'une autre PR, c'est un TODO ou elle est déjà créé? Sinon quel sont les critères pour désactiver le "follow position"? On pourrait faire des propositions pour corriger le problème.
Please check if the PR fulfills these requirements
What is the current behavior? (You can also link to an open issue here)
Panning the view does not desactivate the follow position. The GPS button only disactivate the tracking of the user on the map.
What is the new behavior?
Panning the view if the follow position was activated, will only temporarily disactivate it. The GPS button will now reactivate the follow position when clicked.
If the user manually disactivate the tracking, the GPS button will only reactivate the tracking without the follow position.
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: