Preview fixes #257
Preview fixes #257
Conversation
Simplifies and uniformizes the calculation of previews indexes, in the different navigation cases (changes of current images, uses of previews arrows). Provides fixes for: - clicking the right image arrow always shifts the displayed previews, even in cases where it shouldn't, for instance for n>=3 and current=0 (root cause of 'clipping' effects when navigating away from first/last preview) - the number of displayed previews sometimes oscillate between n (number of requested previews in config) and n-1/n+1 while navigating (for example for n=4 or n=5) - when opening the modal and navigating to the last preview by clicking on the right preview arrow, it's impossible to then click on the left preview arrow (same from last to first and clicking on the right preview arrow)
In infinite sliding, the left and right preview arrows should be always visible (except if nbPreviews < nbImages)
| @@ -738,7 +738,15 @@ describe('PreviewsComponent', () => { | |||
|
|
|||
| previews = element.queryAll(By.css('img')); | |||
|
|
|||
| previews[1].nativeElement.click(); | |||
There was a problem hiding this comment.
Addresses: 1. clicking the right image arrow always shifts the displayed previews, even in cases where it shouldn't, for instance for nbPreviews>=3 and current=0
| @@ -853,6 +861,136 @@ describe('PreviewsComponent', () => { | |||
| checkPreview(previews[i], previewImages[i], i === 0, DEFAULT_PREVIEW_SIZE); | |||
| } | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
Addresses: 2. the number of previews oscillate between n (number of requested previews) and n-1/n+1, for example when n=4 or n=5
| })); | ||
| }); | ||
|
|
||
| it('should allow to navigate previews from first to last and back', () => { |
There was a problem hiding this comment.
Addresses: when opening the modal and navigating to the last preview by clicking on the right preview arrow, it's impossible to then click on the left preview arrow: isPreventSliding() prevents it
| }); | ||
|
|
||
| it('should always display previews navigation arrows, in infinite sliding mode (nbPreviews < nbImages)', () => { | ||
| const configService = fixture.debugElement.injector.get(ConfigService); |
There was a problem hiding this comment.
Addresses: 4. in infinite sliding, the left and right preview arrows should be always visible (like the left and right image arrows), except if nbPreviews < images.length
projects/ks89/angular-modal-gallery/src/lib/components/previews/previews.html
Show resolved
Hide resolved
| @@ -170,28 +170,12 @@ export class PreviewsComponent extends AccessibleComponent implements OnInit, On | |||
| * In particular, it's called when any data-bound property of a directive changes!!! | |||
| */ | |||
| ngOnChanges(changes: SimpleChanges): void { | |||
There was a problem hiding this comment.
In order for the previews calculation to be more consistent and simple, I simplified this part so that the previews are calculated, with any change, by the same process.
| */ | ||
| private initPreviews(currentImage: InternalLibImage, images: InternalLibImage[]): void { |
There was a problem hiding this comment.
Same here: for simplification and edge-case handling, I propose that the whole logic be processed uniformly in a single method (setIndexesPreviews)
projects/ks89/angular-modal-gallery/src/lib/components/previews/previews.component.ts
Show resolved
Hide resolved
| throw new Error('Internal library error - images must be defined'); | ||
| } | ||
| // check if nextImage should be blocked | ||
| if (this.isPreventSliding(this.images.length - 1)) { |
There was a problem hiding this comment.
preventSliding depends whether next() or previous() is called, so I moved the isPreventSliding logic within these two methods for better contextualization.
|
|
||
| this.start++; | ||
| this.end = Math.min(this.end + 1, this.images.length); | ||
| this.end = this.start + Math.min((this.previewConfig.number as number), this.images.length); |
There was a problem hiding this comment.
Also, start and end are calculated in a co-dependant way, like in setIndexesPreviews().
|
|
||
| this.start = Math.max(this.start - 1, 0); | ||
| this.end = Math.min(this.end - 1, this.images.length); | ||
| this.start = this.end - Math.min((this.previewConfig.number as number), this.images.length); |
There was a problem hiding this comment.
Also, start and end are calculated in a co-dependant way, like in setIndexesPreviews().
| throw new Error('Internal library error - images must be defined'); | ||
| } | ||
| // check if prevImage should be blocked | ||
| if (this.isPreventSliding(0)) { |
There was a problem hiding this comment.
preventSliding depends whether next() or previous() is called, so I moved the isPreventSliding logic within these two methods for better contextualization.
| * @param number boundaryIndex is the first or the last index of `images` input array | ||
| * @returns boolean if true block sliding, otherwise not | ||
| */ | ||
| private isPreventSliding(boundaryIndex: number): boolean { |
There was a problem hiding this comment.
preventSliding depends whether next() or previous() is called, so I moved the isPreventSliding logic within these two methods for better contextualization.
| /** | ||
| * Private method to handle navigation changing the previews array and other variables. | ||
| */ | ||
| private updatePreviews(prev: InternalLibImage, current: InternalLibImage): void { |
There was a problem hiding this comment.
all the calculation is now done in initPreviews to avoid divergence of logics
|
Very cool, I reviewed everything and merged. |
|
released as 9.1.0-beta.2 I made some tests and it seems to be ok. The only missing thing before the stable release is to update doc website with previews template feature. |
|
Awesome ! |
(recreated from latest develop)
(fix #253)
Hi there !
This is quite an important PR about previews. Please rest assured that I've worked on it and tested it quite thoroughly.
Each fixes is accompanied with corresponding tests, which fails on versions before the PR and are green with the PR.
Feel free to review these "specs" first, to check if they're coherent with your vision.
This PR adresses different small quirks:
clicking the right image arrow always shifts the displayed previews, even in cases where it shouldn't, for instance for nbPreviews>=3 and current=0
That's what causes the flickering. The previews are then re-adjusted when 'images' are detected as changed by ngOnChanges; this was making the bug subtle and difficult to see/catch.
the number of previews oscillate between n (number of requested previews) and n-1/n+1, for example when n=4 or n=5
when opening the modal and navigating to the last preview by clicking on the right preview arrow, it's impossible to then click on the left preview arrow: isPreventSliding() prevents it
in infinite sliding, the left and right preview arrows should be always visible (like the left and right image arrows), except if nbPreviews < images.length
This is not a clear specification of your tool, but I've imagined it would be consistent.
I'll annotate the .spec file in the PR, so you can check which test addresses which point.
I'll also annotate the code in the PR, so you can understand the reason I had to refactor.