Feature/image image component by saramaenza · Pull Request #164 · evt-project/evt-viewer-angular

saramaenza · 2021-12-04T15:53:21Z

No description provided.

src/app/components/osd/osd.component.ts

szenzaro · 2021-12-07T10:29:35Z

src/app/panels/image-panel/image-panel.component.html

-            [manifestURL]="manifest">
+            [manifestURL]="manifest"
+            [page]="imageIndex"
+            (data)="eventHandler($event)"


Since you are vinding to page change event use that instead.

page change event does not consider the page change through arrows

make the buttons fire that event instead

src/app/panels/image-panel/image-panel.component.ts

szenzaro · 2021-12-07T10:31:35Z

src/app/panels/image-panel/image-panel.component.ts

+      });
+      this._imageIndex = 0;
+      this.currentImg = this.images[this.page].label;
+      this.cdref.detectChanges();


Why do you call explicitly change detection?

I added change detection because otherwise the error "ERROR Error: NG0100: ExpressionChangedAfterItHasBeenCheckedError: Expression has changed after it was checking" would appear in the console, since every time the image name is updated without the page being refreshed

Usually such errors is due to an improper initialisation.

szenzaro · 2021-12-07T10:32:59Z

src/app/panels/image-panel/image-panel.component.ts

+    }
+  }
+
+  isMsDescOpen(event: boolean){


This should be just this.msDescOpen why do you need the other lines?

Weird to have a check on the internal state that depends on an event that is a boolean.
Check this function again, it is probably overcomplicated.

szenzaro · 2021-12-07T10:34:49Z

src/app/panels/image-panel/image-panel.component.ts

+    }
+  }
+
+  setMsDescID(event: string){


Check naming, of parameters, you call event boolean, strings and actual events

this could be a setter

szenzaro · 2021-12-07T10:36:59Z

src/app/view-modes/image-image/image-image.component.ts

+  }
+
+  private initGridster() {
+    this.layoutOptions = {


you can put this at line 10.

If you move it at line 15 (15 is the new 10 XD), you can remove the method initGridster

szenzaro · 2021-12-07T10:37:23Z

src/app/view-modes/image-image/image-image.component.ts

+  public imagePanel2Item: GridsterItem = { cols: 1, rows: 1, y: 0, x: 1 };
+
+  ngOnInit() {
+    this.initGridster();


Do you need to set layout options here or is it enough to set at construction time?

You can probably move it in the constructor!

szenzaro · 2022-01-11T16:39:04Z

src/app/components/osd/osd.component.ts

    if (v !== this._page) {
      this._page = v;
-      this.pageChange.next(this._page);
+      this.pageChange.next(this._page + 1);


Why the +1?

@saramaenza can answer to @szenzaro? Why do you need the +1?

src/app/components/osd/osd.component.ts

src/app/panels/image-panel/image-panel.component.ts

src/app/panels/image-panel/image-panel.component.html

ChiaraDipi

Since you added a new icon, you should update also the file evt-icons-project.json into src/assets/fonts, otherwise new future icons addition will override yours!

ChiaraDipi · 2022-07-10T15:32:57Z

src/app/components/osd/osd.component.ts

    if (v !== this._page) {
      this._page = v;
-      this.pageChange.next(this._page);
+      this.pageChange.next(this._page + 1);


@saramaenza can answer to @szenzaro? Why do you need the +1?

ChiaraDipi · 2022-07-10T15:33:07Z

src/app/components/osd/osd.component.ts

  ngOnDestroy(): void {
    this.subscriptions.forEach((s) => s.unsubscribe());
  }
+


Remove this line

ChiaraDipi · 2022-07-10T15:34:39Z

src/app/panels/image-panel/image-panel.component.html

    <div header-left>
-        <!-- TODO: Add dropdowns for navigation -->
-        <evt-ms-desc-selector #msDesc (selectionChange)="setMsDescID($event)" (msDescOpen)="setMsDescOpen($event)"></evt-ms-desc-selector>
+    <ng-select 


Can you restore the TODO comment?

ChiaraDipi · 2022-07-10T15:39:18Z

src/app/panels/image-panel/image-panel.component.html

-        <div *ngIf="msDescOpen">
-            <evt-ms-desc [data]="currentMsDesc$ | async"></evt-ms-desc>    
+        <div *ngIf="(msDesc$ | async) as msDescs">
+            <div *ngFor="let msDesc of msDescs">


You can use the async binding directly in the loop:

<div *ngFor="let msDesc of msDescs$ | async">

FYI, if you need to add a level to handle logic (like you did here), but don't need additional HTML you can use ng-container.

ChiaraDipi · 2022-07-10T15:40:13Z

src/app/panels/image-panel/image-panel.component.ts

+  private _imageIndex: number;
+  public manifest;
+  public imagePath;
+  public filename;


Move all these property before the contructor.

ChiaraDipi · 2022-07-10T15:43:05Z

src/app/panels/image-panel/image-panel.component.ts

+  public imagePath;
+  public filename;
+
+  toggleImg(event){


Add space before "{"

ChiaraDipi · 2022-07-10T15:47:05Z

src/app/panels/image-panel/image-panel.component.ts

+    else {
+      this.currentImg = 'Error loading';
+    }
  }


All the logic here depends on viewerData. You can try to change the @Input prop into a setter/getter and move this logic inside it.

private _vd: ViewerDataType; @Input() set viewerData(vd: ViewerDataType) { // Your logic goes here } get viewerData() { return this._vd; }

NB: The getter is only needed if you access viewerData elsewhere, otherwise you can omit it.

ChiaraDipi · 2022-07-10T15:49:27Z

src/app/panels/image-panel/image-panel.component.ts

+    return this.showSecondaryContent;
  }
+
+  isMsDescOpen(event: boolean){


The name of this method suggests a return value. If you don't need a return value probably you should change the name of the method.

ChiaraDipi · 2022-07-10T15:51:23Z

src/app/view-modes/image-image/image-image.component.ts

+  public imagePanel2Item: GridsterItem = { cols: 1, rows: 1, y: 0, x: 1 };
+
+  ngOnInit() {
+    this.initGridster();


You can probably move it in the constructor!

ChiaraDipi · 2022-07-10T15:52:58Z

src/app/view-modes/image-image/image-image.component.ts

+  }
+
+  private initGridster() {
+    this.layoutOptions = {


If you move it at line 15 (15 is the new 10 XD), you can remove the method initGridster

saramaenza requested review from ChiaraDipi, giuliac89 and szenzaro December 4, 2021 15:53

szenzaro requested changes Dec 7, 2021

View reviewed changes

saramaenza force-pushed the feature/image-image-component branch from 96ab1fc to 3e05ba6 Compare January 5, 2022 18:46

szenzaro requested changes Jan 11, 2022

View reviewed changes

saramaenza force-pushed the feature/image-image-component branch from 22265c0 to 43dc59f Compare January 26, 2022 09:21

saramaenza force-pushed the feature/image-image-component branch from 43dc59f to 477c9a0 Compare February 26, 2022 14:56

SaraMaenza added 22 commits May 30, 2022 19:16

Add image-image component

32cf647

Add image-image-component in app.module

4e961af

Update image-image template

ad2f657

Add image-image component in app-routing-module

4cd43fd

Add image-image icon in edition_config

fdfa59b

Update image-panel template

532a659

Update image-panel component

1f22665

Update image-image component

729200e

Update osd component

240847e

Update image-panel template

2be71e3

Update image-panel component

0954320

Update image-panel template

e079d3a

Update osd component

c758c7d

Add imgImg icon in icon style component

cc4d663

Update fonts in assets

7914c1a

Add msDesc selector in image panel template

79d7175

Update image-image component

d71f043

Update osd component

068157b

Update image-panel template

ed7f86f

Add width in ng-select

2b0bd39

Add width to the image selector

6acd64e

Delete pageImg output from osd component

b9db170

Update changelog

8aeff48

saramaenza force-pushed the feature/image-image-component branch from 11ba834 to 8aeff48 Compare May 30, 2022 17:46

SaraMaenza added 10 commits May 30, 2022 19:50

Delete manifestResourcetoTileSource function

9553d1f

Update image-panel component template

48ef7d0

Add comma in image-panel component

3f61923

Update image-image component template

941d331

Update image-panel component

10a1a62

Update image-image component

82e5cbf

Add comma in image-text component

33664d9

Update image-panel template

5135637

Update image-panel component

285085e

Update image-panel component

439fe69

ChiaraDipi requested changes Jul 10, 2022

View reviewed changes

Conversation

saramaenza commented Dec 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChiaraDipi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants