feat(geo): Added rotate option and north direction to the map when pr… #1256

aziz-access · 2023-05-16T13:57:53Z

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")

Yes
No

…inting

pelord · 2023-05-16T19:02:25Z

@aziz-access You know that there is already a compoment for rotation? ..\packages\geo\src\lib\map\rotation-button

aziz-access · 2023-05-16T19:11:50Z

@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.

…ntWithRotation

alecarn · 2023-05-24T19:38:01Z

packages/geo/src/lib/map/rotation-button/rotation-button.component.ts

@@ -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');


Une manière sans avoir à utiliser le ElementRef mais c'est bon aussi comme tu le fais.

Suggested change

elRef.nativeElement.classList.add('north-direction', 'ol-unselectable');

@HostBinding('class') hostClass = 'north-direction ol-unselectable';

alecarn · 2023-05-24T19:44:08Z

packages/geo/src/lib/map/shared/map.ts

-    const currentControls = Object.assign([], this.ol.getControls().getArray());
-    currentControls.forEach(control => {
-      this.ol.removeControl(control);
-    });


Peux-tu m'expliquer la raison des changements dans ce fichier?

packages/geo/src/lib/print/shared/print.service.ts

alecarn · 2023-05-24T19:58:14Z

packages/geo/src/lib/print/shared/print.service.ts

-    margins: Array<number>
+    margins: Array<number>,
+    size: Array<number>,
+    position: string,


On devrait utiliser l'enum de PrintLegendPosition

Suggested change

position: string,

position: PrintLegendPosition,

alecarn · 2023-05-26T14:08:16Z

packages/geo/src/lib/map/rotation-button/rotation-button.component.ts

@@ -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';


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 et north-direction-reset

Pourquoi ajouter la classe ol-unselectable?

alecarn · 2023-05-26T14:13:20Z

packages/geo/src/lib/print/shared/print.service.ts

-      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;


Suggested change

const northDirection = mapOverlayHTML.getElementsByClassName('north-direction')[0] as HTMLElement;

const northDirection = mapOverlayHTML.getElementsByTagName("igo-rotation-button")[0] as HTMLElement;

alecarn · 2023-05-26T18:15:45Z

packages/geo/src/lib/print/shared/print.service.ts

+          rotateNorth.style.width = 'inherit';
+          rotateNorth.style.paddingLeft = '10px';
+        }
+      }


Code dupliquer, analyser si on peut supprimer

alecarn · 2023-05-26T18:20:18Z

packages/geo/src/lib/print/shared/print.service.ts

    ) {
      const context = canvas.getContext('2d');
      let canvasOverlayHTML;
-      const mapOverlayHTML = map.ol.getOverlayContainerStopEvent();
+      const mapOverlayHTML = map.ol.getOverlayContainerStopEvent().cloneNode(true) as HTMLElement;


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

Suggested change

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;

}

packages/geo/src/lib/print/shared/print.service.ts

alecarn · 2023-05-26T19:30:31Z

packages/geo/src/lib/print/shared/print.service.ts

-      if (olCollapsed) {
-        element.classList.add('ol-collapsed');
-      }
+      document.getElementById('print-area').remove();


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.

Suggested change

document.getElementById('print-area').remove();

this.removeDomElement(mapOverlayHTML);

private removeDomElement(element: HTMLElement): void {

if (!element.parendNode) { return; }

element.parentNode.removeChild(element);

}

alecarn · 2023-05-26T19:37:23Z

packages/geo/src/lib/print/shared/print.service.ts

+      mapContextResult.drawImage(mapCanvas, 0, 0);
+      mapContextResult.globalAlpha = 1;
+      // reset canvas transform to initial
+      mapContextResult.setTransform(1, 0, 0, 1, 0, 0);


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?

…ntWithRotation

alecarn

Quelques commentaires mineures, good job c'était un bon ménage.

alecarn · 2023-06-01T11:57:16Z

packages/geo/src/lib/print/shared/print.service.ts

@@ -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];


Inférer le type as HTMLCanvasElement

alecarn · 2023-06-01T12:11:24Z

packages/geo/src/lib/print/shared/print.service.ts

+   */
+  private async drawMap(
+    size: Array<number>,
+    mapCanvas: any


Typer, HTMLCanvasElement

alecarn · 2023-06-01T12:28:58Z

packages/geo/src/lib/print/shared/print.service.ts

+   * @param mapOverlayHTML mapOverlay
+   * @param position - legend Position
+   * @returns - HTMLElement | null
+   */


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é

alecarn · 2023-06-01T12:30:47Z

packages/geo/src/lib/print/shared/print.service.ts

+   * @param position - legend Position
+   * @returns - HTMLElement | null
+   */
+  private async setUpNorthDirection(mapOverlayHTML: HTMLElement, position: PrintLegendPosition): Promise<HTMLElement | null> {


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 pour addNorthDirection 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

alecarn · 2023-06-01T12:47:04Z

packages/geo/src/lib/print/shared/print.service.ts

+      mapResultCanvas.height = size[1];
+      const opacity = mapCanvas.parentNode.style.opacity || mapCanvas.style.opacity;
+      mapContextResult.globalAlpha = opacity === '' ? 1 : Number(opacity);
+      let matrix;


Typer matrix: number[]

packages/geo/src/lib/print/shared/print.service.ts

alecarn · 2023-06-01T12:59:19Z

packages/geo/src/lib/print/shared/print.service.ts

+      await html2canvas(mapOverlayHTML, {
+        scale: 1,
+        backgroundColor: null,
+        allowTaint: true,
+        useCORS: true,
+      }).then( e => {
+        canvasOverlayHTML = e;
+      });


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

Suggested change

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,

});

packages/geo/src/lib/print/shared/print.service.ts

alecarn · 2023-06-01T13:28:18Z

packages/geo/src/lib/print/shared/print.service.ts

+   * @param mapOverlayHTML
+   * @returns mapOverlay
+   */
+  private async setUpAttribution(mapOverlayHTML: HTMLElement): Promise<HTMLElement | null> {


Good job Aziz, c'est bien segmenté pour le addNorth et addAttribution

alecarn · 2023-06-01T13:32:51Z

packages/geo/src/lib/print/shared/print.service.ts

        let status = SubjectStatus.Done;
        try {
-          for (const canvas of canvases) {


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.

ici nous n'avons qu'une seule canvas pour la carte c'est pourquoi j'ai supprimer la boucle.
@pelord pouvez-vous confirmer cela?

alecarn

Quelques commentaires mineures, good job c'était un bon ménage.

feat(geo): Added rotate option and north direction to the map when pr…

c90d559

…inting

aziz-access added enhancement New features and improvements // Nouvelles fonctionnalités et améliorations in progress In progress // Développement en cours labels May 16, 2023

aziz-access self-assigned this May 16, 2023

aziz-access added 7 commits May 16, 2023 21:51

solve lint and optimize map updateControls function

1346b9a

integrate custom controle rotation-button component in the map

083eeaa

adjust image in all template

6233434

solve scale and rotate arrow color and position in pdf export

7d6c6aa

code improvement and write documentations

bb4bc80

sove lint

c134874

Merge branch 'next' of github.com:infra-geo-ouverte/igo2-lib into pri…

9d8084a

…ntWithRotation

alecarn self-requested a review May 24, 2023 18:02

alecarn reviewed May 24, 2023

View reviewed changes

code improvement

7af119d

alecarn requested changes May 26, 2023

View reviewed changes

alecarn linked an issue May 29, 2023 that may be closed by this pull request

Impression + rotation infra-geo-ouverte/igo2#775

Closed

alecarn mentioned this pull request May 29, 2023

feat(geo): Print with resolution PDF and image #1264

Merged

2 tasks

aziz-access added 6 commits May 29, 2023 22:41

code improvement eliminate repeated code

32f8df9

solve lint

2abec17

code refactoring

9ded749

Merge branch 'next' of github.com:infra-geo-ouverte/igo2-lib into pri…

7741950

…ntWithRotation

solve lint

279a35b

delete mapOverlayHTML from dom

afdb476

alecarn requested changes Jun 1, 2023

View reviewed changes

aziz-access added 3 commits June 1, 2023 19:07

solve comments

973d2aa

add type to map canvas

93c94b3

delete console log

aa1f48f

alecarn requested a review from pelord June 2, 2023 11:35

alecarn approved these changes Jun 2, 2023

View reviewed changes

alecarn removed the request for review from pelord June 2, 2023 15:20

alecarn merged commit b79c7a0 into next Jun 5, 2023
5 checks passed

alecarn deleted the printWithRotation branch June 5, 2023 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(geo): Added rotate option and north direction to the map when pr… #1256

feat(geo): Added rotate option and north direction to the map when pr… #1256

aziz-access commented May 16, 2023

pelord commented May 16, 2023

aziz-access commented May 16, 2023 •

edited

alecarn May 24, 2023

alecarn May 24, 2023

alecarn May 24, 2023

alecarn May 26, 2023

alecarn May 26, 2023

alecarn May 26, 2023

alecarn May 26, 2023

alecarn May 26, 2023

alecarn May 26, 2023

alecarn left a comment

alecarn Jun 1, 2023

alecarn Jun 1, 2023

alecarn Jun 1, 2023

alecarn Jun 1, 2023

alecarn Jun 1, 2023

alecarn Jun 1, 2023

alecarn Jun 1, 2023

alecarn Jun 1, 2023

aziz-access Jun 1, 2023

alecarn left a comment

	elRef.nativeElement.classList.add('north-direction', 'ol-unselectable');
	@HostBinding('class') hostClass = 'north-direction ol-unselectable';

	const northDirection = mapOverlayHTML.getElementsByClassName('north-direction')[0] as HTMLElement;
	const northDirection = mapOverlayHTML.getElementsByTagName("igo-rotation-button")[0] as HTMLElement;

-      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;
+      }

-      document.getElementById('print-area').remove();
+      this.removeDomElement(mapOverlayHTML);
+      private removeDomElement(element: HTMLElement): void {
+        if (!element.parendNode) { return; }
+        element.parentNode.removeChild(element);
+      }

feat(geo): Added rotate option and north direction to the map when pr… #1256

feat(geo): Added rotate option and north direction to the map when pr… #1256

Conversation

aziz-access commented May 16, 2023

pelord commented May 16, 2023

aziz-access commented May 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecarn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecarn left a comment

Choose a reason for hiding this comment

aziz-access commented May 16, 2023 •

edited