-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat(geo): Added rotate option and north direction to the map when pr… #1256
Conversation
@aziz-access You know that there is already a compoment for rotation? ..\packages\geo\src\lib\map\rotation-button |
@pelord yes i know that component, but not exist in OverlayContainer of the map, so i add a new control 'rotation' for the map to add "ol-rotation" in the OverlayContainer. |
@@ -21,9 +22,17 @@ export class RotationButtonComponent implements AfterContentInit { | |||
@Input() showIfNoRotation: boolean; | |||
@Input() color: string; | |||
|
|||
constructor() { } | |||
constructor(private elRef: ElementRef) { | |||
elRef.nativeElement.classList.add('north-direction', 'ol-unselectable'); |
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.
Une manière sans avoir à utiliser le ElementRef mais c'est bon aussi comme tu le fais.
elRef.nativeElement.classList.add('north-direction', 'ol-unselectable'); | |
@HostBinding('class') hostClass = 'north-direction ol-unselectable'; |
const currentControls = Object.assign([], this.ol.getControls().getArray()); | ||
currentControls.forEach(control => { | ||
this.ol.removeControl(control); | ||
}); |
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.
Peux-tu m'expliquer la raison des changements dans ce fichier?
margins: Array<number> | ||
margins: Array<number>, | ||
size: Array<number>, | ||
position: 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 devrait utiliser l'enum de PrintLegendPosition
position: string, | |
position: PrintLegendPosition, |
@@ -20,10 +21,18 @@ export class RotationButtonComponent implements AfterContentInit { | |||
@Input() map: IgoMap; | |||
@Input() showIfNoRotation: boolean; | |||
@Input() color: string; | |||
@HostBinding('class') hostClass = 'north-direction ol-unselectable'; |
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 me questionne sur la raison de l'ajout de ces classes.
- Pourquoi ajouter une classe supplémentaire alors que ça me semble seulement pour ajouter un identifiant.
Voir le commentaire pour une alternative et on pourrait supprimer cette classe etnorth-direction-reset
- Pourquoi ajouter la classe
ol-unselectable
?
const mapOverlayHTML = map.ol.getOverlayContainerStopEvent(); | ||
const mapOverlayHTML = map.ol.getOverlayContainerStopEvent().cloneNode(true) as HTMLElement; | ||
mapOverlayHTML.id = 'print-area'; | ||
const northDirection = mapOverlayHTML.getElementsByClassName('north-direction')[0] as HTMLElement; |
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.
const northDirection = mapOverlayHTML.getElementsByClassName('north-direction')[0] as HTMLElement; | |
const northDirection = mapOverlayHTML.getElementsByTagName("igo-rotation-button")[0] as HTMLElement; |
rotateNorth.style.width = 'inherit'; | ||
rotateNorth.style.paddingLeft = '10px'; | ||
} | ||
} |
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.
Code dupliquer, analyser si on peut supprimer
) { | ||
const context = canvas.getContext('2d'); | ||
let canvasOverlayHTML; | ||
const mapOverlayHTML = map.ol.getOverlayContainerStopEvent(); | ||
const mapOverlayHTML = map.ol.getOverlayContainerStopEvent().cloneNode(true) as HTMLElement; |
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.
Dans la méthode addScale
, on redimensionne l'overlay mais ici on ne le fait pas pourquoi? Si on a besoin de toujours redimensionner, on pourrait faire une méthode réutilisable getMapOverlay
const mapOverlayHTML = map.ol.getOverlayContainerStopEvent().cloneNode(true) as HTMLElement; | |
const mapOverlayHTML = this.getMapOverlay(map, size, resolution); | |
Par exemple: | |
private getMapOverlay(map: IgoMap, size: Array<number>, resolution: number): HTMLElement { | |
const printWidthPixels = Math.round((size[0] * resolution) / 25.4); | |
const printHeightPixels = Math.round((size[1] * resolution) / 25.4); | |
const mapOverlayHTML = map.ol.getOverlayContainerStopEvent().cloneNode(true) as HTMLElement; | |
mapOverlayHTML.style.width = printWidthPixels+'px'; | |
mapOverlayHTML.style.height = printHeightPixels+'px'; | |
return mapOverlayHTML; | |
} |
if (olCollapsed) { | ||
element.classList.add('ol-collapsed'); | ||
} | ||
document.getElementById('print-area').remove(); |
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 supprimer l'élément en utilisant directement la référence mapOverlayHTML
. Ainsi on aurrait plus de besoin de référencer nos éléments dom avec des id.
document.getElementById('print-area').remove(); | |
this.removeDomElement(mapOverlayHTML); | |
private removeDomElement(element: HTMLElement): void { | |
if (!element.parendNode) { return; } | |
element.parentNode.removeChild(element); | |
} |
mapContextResult.drawImage(mapCanvas, 0, 0); | ||
mapContextResult.globalAlpha = 1; | ||
// reset canvas transform to initial | ||
mapContextResult.setTransform(1, 0, 0, 1, 0, 0); |
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.
Pourrais-tu m'expliquer pourquoi nous avons besoin de tous ces changements pour rajouter le bouton de rotation sur la carte? Le code semble en partie dupliquer à partir de la ligne 902 ci-dessous?
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.
Quelques commentaires mineures, good job c'était un bon ménage.
@@ -860,7 +873,8 @@ export class PrintService { | |||
map.ol.once('rendercomplete', async (event: any) => { | |||
const size = map.ol.getSize(); | |||
const mapCanvas = event.target.getViewport().getElementsByTagName('canvas')[0]; |
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.
Inférer le type as HTMLCanvasElement
*/ | ||
private async drawMap( | ||
size: Array<number>, | ||
mapCanvas: any |
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.
Typer, HTMLCanvasElement
* @param mapOverlayHTML mapOverlay | ||
* @param position - legend Position | ||
* @returns - HTMLElement | null | ||
*/ |
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.
C'est un sujet très subjectif mais pourquoi ajouter ces commentaires?
Pour moi un commentaire c'est une exception parce que le code devrait être lisible et auto explicatif. Si le code n'est pas clair et qu'on doit ajouter des commentaires, c'est un signe qu'il faut retourner a la planche a dessin et raffiner notre code, segmenter pour qu'il soit lisible. Un commentaire, c'est de la maintenance en plus et souvent les développeurs oublie de les maintenir. Bref, le code ne ment pas mais un commentaire a cette possibilité.
Pour moi ta méthode sans commentaire est bien lisible on a toute l'info qu'on a besoin pour comprendre
- setUpNorthDirection: un nom révélateur
- Les paramètres de la méthodes sont claire
- Le retour est typer!
Voir la ligne 588 pour un commentaire qui n'est pas maintenu et qui propage de la fausse information du au changement. // img is corrupt todo: fix addScale in mobile make a corrupt img
. La méthode addScale
a été supprimé
* @param position - legend Position | ||
* @returns - HTMLElement | null | ||
*/ | ||
private async setUpNorthDirection(mapOverlayHTML: HTMLElement, position: PrintLegendPosition): Promise<HTMLElement | null> { |
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.
Ce commentaire s'applique aux autres méthodes similaires.
- Est-ce qu'il y a un typo dans le nom de la méthode
set up
j'irais pouraddNorthDirection
comme tu l'as mentionné dans ton commentaire de la méthode - Pour le retour je m'attendrais a un
Promise<void>
. Dans le code on utilise nul part le retour de cette méthode
mapResultCanvas.height = size[1]; | ||
const opacity = mapCanvas.parentNode.style.opacity || mapCanvas.style.opacity; | ||
mapContextResult.globalAlpha = opacity === '' ? 1 : Number(opacity); | ||
let matrix; |
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.
Typer matrix: number[]
await html2canvas(mapOverlayHTML, { | ||
scale: 1, | ||
backgroundColor: null, | ||
allowTaint: true, | ||
useCORS: true, | ||
}).then( e => { | ||
canvasOverlayHTML = e; | ||
}); |
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.
Essayer d'éviter de chainer les promesses lorsqu'on utilise les Async/Await. Je trouve que c'est beaucoup plus lisible ainsi et on pourra rapporter let canvasOverlayHTML;
dans son contexte au lieu d'être orphelin dans le haut de la méthode
await html2canvas(mapOverlayHTML, { | |
scale: 1, | |
backgroundColor: null, | |
allowTaint: true, | |
useCORS: true, | |
}).then( e => { | |
canvasOverlayHTML = e; | |
}); | |
const canvasOverlayHTML = await html2canvas(mapOverlayHTML, { | |
scale: 1, | |
backgroundColor: null, | |
allowTaint: true, | |
useCORS: true, | |
}); |
* @param mapOverlayHTML | ||
* @returns mapOverlay | ||
*/ | ||
private async setUpAttribution(mapOverlayHTML: HTMLElement): Promise<HTMLElement | null> { |
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.
Good job Aziz, c'est bien segmenté pour le addNorth et addAttribution
let status = SubjectStatus.Done; | ||
try { | ||
for (const canvas of canvases) { |
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 avoir supprimer cette itération, dans quel cas de figure qu'on a plusieurs Canvas dans Openlayers? J'ai le feeling qu'on pourrait avoir une régression dans des cas spécifiques d'impression.
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.
ici nous n'avons qu'une seule canvas pour la carte c'est pourquoi j'ai supprimer la boucle.
@pelord pouvez-vous confirmer cela?
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.
Quelques commentaires mineures, good job c'était un bon ménage.
What is the current behavior? (You can also link to an open issue here)
this behavior related to #775 in igo2
What is the new behavior?
Improved printing when rotating the map and adding north direction
Does this PR introduce a breaking change? (check one with "x")