-
Notifications
You must be signed in to change notification settings - Fork 20
Feature/image image component #164
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
base: develop
Are you sure you want to change the base?
Changes from all commits
32cf647
4e961af
ad2f657
4cd43fd
fdfa59b
532a659
1f22665
729200e
240847e
2be71e3
0954320
e079d3a
c758c7d
cc4d663
7914c1a
79d7175
d71f043
068157b
ed7f86f
2b0bd39
6acd64e
b9db170
8aeff48
9553d1f
48ef7d0
3f61923
941d331
10a1a62
82e5cbf
33664d9
5135637
285085e
439fe69
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,7 +100,7 @@ export class OsdComponent implements AfterViewInit, OnDestroy { | |
| @Input() set page(v: number) { | ||
| if (v !== this._page) { | ||
| this._page = v; | ||
| this.pageChange.next(this._page); | ||
| this.pageChange.next(this._page + 1); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -174,4 +174,5 @@ export class OsdComponent implements AfterViewInit, OnDestroy { | |
| ngOnDestroy(): void { | ||
| this.subscriptions.forEach((s) => s.unsubscribe()); | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this line |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,33 @@ | ||
| <evt-panel [showSecondaryContent]="msDescOpen" | ||
| [hideHeader]="false" | ||
| [hideFooter]="true"> | ||
| <evt-panel [showSecondaryContent]="isSecondaryContentOpened()"> | ||
| <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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you restore the TODO comment? |
||
| class="mr-1 imgSelect" | ||
| bindLabel="label" | ||
| bindValue="label" | ||
| [searchable]="true" | ||
| [clearable]="false" | ||
| [ngModel]="currentImg" | ||
| (change)="toggleImg($event)"> | ||
| <ng-option [value]="item" *ngFor="let item of images;"> | ||
| {{ item.label || item.url.split('/').pop().split('.')[0] }} | ||
| </ng-option> | ||
| </ng-select> | ||
| <evt-ms-desc-selector (selectionChange)="setMsDescID($event)" (msDescOpen)="isMsDescOpen($event)"></evt-ms-desc-selector> | ||
| </div> | ||
| <div content> | ||
| <evt-osd | ||
| *ngIf="viewerData" | ||
| [viewerData]="viewerData"> | ||
| [viewerData]="viewerData" | ||
| [page]="imageIndex" | ||
| (pageChange)="eventPageImg($event)"> | ||
| </evt-osd> | ||
| <p *ngIf="!viewerData">Found no source file</p> | ||
| </div> | ||
|
|
||
| <div secondary-content> | ||
| <div *ngIf="msDescOpen"> | ||
| <evt-ms-desc [data]="currentMsDesc$ | async"></evt-ms-desc> | ||
| <div *ngIf="(msDesc$ | async) as msDescs"> | ||
| <div *ngFor="let msDesc of msDescs"> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use the async binding directly in the loop: FYI, if you need to add a level to handle logic (like you did here), but don't need additional HTML you can use |
||
| <evt-ms-desc *ngIf="msDesc.id == msDescID" [data]="msDesc"></evt-ms-desc> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </evt-panel> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,5 @@ | ||
| import { Component, Input } from '@angular/core'; | ||
| import { BehaviorSubject, combineLatest } from 'rxjs'; | ||
| import { filter, map } from 'rxjs/operators'; | ||
| import { HttpClient } from '@angular/common/http'; | ||
| import { ChangeDetectorRef, Component, Input } from '@angular/core'; | ||
| import { ViewerDataType } from '../../models/evt-models'; | ||
| import { EVTModelService } from '../../services/evt-model.service'; | ||
|
|
||
|
|
@@ -12,24 +11,74 @@ import { EVTModelService } from '../../services/evt-model.service'; | |
| export class ImagePanelComponent { | ||
| @Input() viewerData: ViewerDataType; | ||
|
|
||
| currentMsDescId$ = new BehaviorSubject(undefined); | ||
| currentMsDesc$ = combineLatest([this.evtModelService.msDesc$, this.currentMsDescId$]).pipe( | ||
| filter(([msDesc, currentId]) => !!msDesc && !!currentId), | ||
| map(([msDesc, currentId]) => msDesc.find(m => m.id === currentId)), | ||
| ); | ||
|
|
||
| msDescOpen = false; | ||
| get imageIndex() { return this._imageIndex; } | ||
|
|
||
| constructor( | ||
| private evtModelService: EVTModelService, | ||
| public evtModelService: EVTModelService, | ||
| public http: HttpClient, | ||
| private cdref: ChangeDetectorRef, | ||
| ) { | ||
| } | ||
| public msDesc$ = this.evtModelService.msDesc$; | ||
| public showSecondaryContent = false; | ||
| public selectedPage; | ||
| public msDescID = ''; | ||
| public value: number; | ||
| public images = []; | ||
| public currentImg: string; | ||
| // tslint:disable-next-line: variable-name | ||
| private _imageIndex: number; | ||
| public manifest; | ||
| public imagePath; | ||
| public filename; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move all these property before the contructor. |
||
|
|
||
| toggleImg(event){ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add space before "{" |
||
| this._imageIndex = this.images.indexOf(event); | ||
| } | ||
|
|
||
| eventPageImg(pageImg: number){ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name of this method is not very explanatory. Can you find another one? |
||
| if (this.viewerData?.type === 'manifest'){ | ||
| this.currentImg = this.images[pageImg - 1].label; | ||
| } | ||
| if (this.viewerData?.type === 'default' || this.viewerData?.type === 'graphics'){ | ||
| this.currentImg = this.images[pageImg - 1].url.split('/').pop().split('.')[0]; | ||
| } | ||
| this.cdref.detectChanges(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you call explicitly change detection?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can try to use Observable for currentImage and let the async method handle it. |
||
| } | ||
|
|
||
| setMsDescOpen(isOpen: boolean) { | ||
| this.msDescOpen = isOpen; | ||
| // tslint:disable-next-line: use-lifecycle-interface | ||
| ngOnChanges() { | ||
| if (this.viewerData?.type === 'manifest'){ | ||
| this.manifest = this.viewerData.value?.manifestURL; | ||
| // tslint:disable-next-line: no-any | ||
| this.http.get(`${this.manifest}`).subscribe((item: any) => { | ||
| item.sequences.forEach((value) => { | ||
| this.images.push(...value.canvases); | ||
| }); | ||
| this.currentImg = this.images[0].label; | ||
| }); | ||
| } | ||
| if (this.viewerData?.type === 'default' || this.viewerData?.type === 'graphics'){ | ||
| this.images = this.viewerData?.value?.xmlImages; | ||
| this.imagePath = this.viewerData?.value?.xmlImages[0]?.url; | ||
| this.filename = this.imagePath.split('/').pop().split('.')[0]; | ||
| this.currentImg = this.filename; | ||
| } | ||
| else { | ||
| this.currentImg = 'Error loading'; | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All the logic here depends on NB: The getter is only needed if you access |
||
|
|
||
| setMsDescID(msDescId: string) { | ||
| this.currentMsDescId$.next(msDescId); | ||
| isSecondaryContentOpened(): boolean { | ||
| return this.showSecondaryContent; | ||
| } | ||
|
|
||
| isMsDescOpen(event: boolean){ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be just
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Weird to have a check on the internal state that depends on an event that is a boolean.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| this.showSecondaryContent = event; | ||
| } | ||
|
|
||
| setMsDescID(idMsDesc: string){ | ||
| this.msDescID = idMsDesc; | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| <gridster [options]="layoutOptions"> | ||
| <gridster-item [item]="imagePanel1Item"> | ||
| <evt-image-panel [viewerData]="imageViewer$ | async"></evt-image-panel> | ||
| </gridster-item> | ||
| <gridster-item [item]="imagePanel2Item"> | ||
| <evt-image-panel [viewerData]="imageViewer$ | async"></evt-image-panel> | ||
| </gridster-item> | ||
| </gridster> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import { ComponentFixture, TestBed } from '@angular/core/testing'; | ||
|
|
||
| import { ImageImageComponent } from './image-image.component'; | ||
|
|
||
| describe('ImageImageComponent', () => { | ||
| let component: ImageImageComponent; | ||
| let fixture: ComponentFixture<ImageImageComponent>; | ||
|
|
||
| beforeEach(async() => { | ||
| await TestBed.configureTestingModule({ | ||
| declarations: [ ImageImageComponent ], | ||
| }) | ||
| .compileComponents(); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| fixture = TestBed.createComponent(ImageImageComponent); | ||
| component = fixture.componentInstance; | ||
| fixture.detectChanges(); | ||
| }); | ||
|
|
||
| it('should create', () => { | ||
| expect(component).toBeTruthy(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| import { Component, OnInit } from '@angular/core'; | ||
| import { DisplayGrid, GridsterConfig, GridsterItem, GridType } from 'angular-gridster2'; | ||
| import { map } from 'rxjs/operators'; | ||
| import { AppConfig } from '../../app.config'; | ||
| import { Surface, ViewerDataType, XMLImagesValues } from '../../models/evt-models'; | ||
| import { ViewerSource } from '../../models/evt-polymorphic-models'; | ||
| import { EVTModelService } from '../../services/evt-model.service'; | ||
|
|
||
| @Component({ | ||
| selector: 'evt-image-image', | ||
| templateUrl: './image-image.component.html', | ||
| styleUrls: ['./image-image.component.scss'], | ||
| }) | ||
| export class ImageImageComponent implements OnInit { | ||
| public layoutOptions: GridsterConfig = {}; | ||
| public imagePanel1Item: GridsterItem = { cols: 1, rows: 1, y: 0, x: 0 }; | ||
| public imagePanel2Item: GridsterItem = { cols: 1, rows: 1, y: 0, x: 1 }; | ||
|
|
||
| public imageViewer$ = this.evtModelService.surfaces$.pipe( | ||
| map((surface) => this.getImageViewerType(AppConfig.evtSettings.files.editionImagesSource, surface)), | ||
| ); | ||
|
|
||
| constructor( | ||
| private evtModelService: EVTModelService, | ||
| ) { | ||
| } | ||
|
|
||
| getImageViewerType(editionImages, surface: Surface[]): ViewerDataType { | ||
| for (const key of Object.keys(editionImages)) { | ||
| if (editionImages[key].enabled) { | ||
| return ViewerSource.getDataType(key, surface); | ||
| } | ||
| } | ||
| const xmlImages: XMLImagesValues[] = []; | ||
| this.evtModelService.pages$.pipe().subscribe( | ||
| (pages) => pages.map(page => xmlImages.push({ url: page.facsUrl }), | ||
| )); | ||
|
|
||
| return { type: 'default', value: { xmlImages } }; | ||
| } | ||
|
|
||
| ngOnInit() { | ||
| this.initGridster(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you need to set layout options here or is it enough to set at construction time?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can probably move it in the constructor! |
||
| } | ||
|
|
||
| private initGridster() { | ||
| this.layoutOptions = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can put this at line 10.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you move it at line 15 (15 is the new 10 XD), you can remove the method |
||
| gridType: GridType.Fit, | ||
| displayGrid: DisplayGrid.None, | ||
| margin: 0, | ||
| maxCols: 2, | ||
| maxRows: 1, | ||
| draggable: { | ||
| enabled: true, | ||
| ignoreContent: true, | ||
| dragHandleClass: 'panel-header', | ||
| }, | ||
| resizable: { | ||
| enabled: false, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,6 +152,10 @@ body { | |
| } | ||
| } | ||
|
|
||
| .imgSelect { | ||
| width: 10rem; | ||
| } | ||
|
|
||
| .overflow-y-auto { | ||
| overflow-y: auto; | ||
| } | ||
|
|
||
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.
Why the
+1?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.
@saramaenza can answer to @szenzaro? Why do you need the
+1?