Improved Animations and bugfix for #4

[Simbaclaws]

Dear @kolkov ,

I've succesfully improved the animations of the plugin by making sure that whatever thumbnail is selected the animation would still be played for the corresponding selection. Including the fix for https://github.com/kolkov/ngx-gallery/issues/4.

However a small new bug arises when using this code. I haven't yet found a solution for it but I doubt it's a big issue. Apparently when using the slide animation there is a small white space between the slides when going to a different slide. This wasn't there before and is created by my pull request. I'm guessing it's got something to do with this code:

  addActive(index, image) {
    setTimeout(()=>{
      if (this.selectedIndex == index) {
        image.classList.add('ngx-gallery-active');
      }
    }, 0);
  }

I had to add a timeout in order to trigger the transition animation for the slide effect for the incomming picture. However since there is a 0 ms delay it causes some white space to appear and I haven't yet figured out how to get rid of it.

I'm looking to fix that bug as well, but for now I think this is already a lot better then what the plugin previously could do.

Please take a look to see whether it is good enough for now.

[codecov[bot]]

Codecov Report

Merging #24 into master will decrease coverage by 1.19%. The diff coverage is 8.47%.

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
- Coverage   38.03%   36.84%   -1.20%     
==========================================
  Files          16       16              
  Lines         723      760      +37     
  Branches      152      159       +7     
==========================================
+ Hits          275      280       +5     
- Misses        411      443      +32     
  Partials       37       37              

Impacted Files	Coverage Δ
...b/ngx-gallery-image/ngx-gallery-image.component.ts	27.04% <5.40%> (-3.96%)	:arrow_down:
projects/gallery/src/lib/ngx-gallery.component.ts	33.58% <10.00%> (-1.38%)	:arrow_down:
...ery-thumbnails/ngx-gallery-thumbnails.component.ts	24.81% <16.66%> (+0.20%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 54f1388...da886b7. Read the comment docs.

[kolkov]

One more issue. Video continues playing on selecting next frame.

[Simbaclaws]

Oh, that is an issue that probably also exists in the current master branch then, oh oh. Tomorrow I'll try to fix both issues.

[Simbaclaws]

Just made sure the video stops playing when switching to a different image or video, it is also reset to the beginning of the video.

I haven't yet found a solution for the white space problem, I find this one hard to solve.

Please let me know when you can merge this pull request so we can close #4 and so I can create a new issue for the white space problem.

[kolkov]

I showed in a comment how you can solve the problem by moving the logic of adding classes to the class method. Try to fix.

[Simbaclaws]

Dear @kolkov, where exactly is this comment? I can't find it in any of my commits. Could you link me to it? I also noticed this github repository tends to have 504 gateway timeouts for some reason. And I just noticed a 500 error on the official github homepage. I guess we'll have to wait until things are fixed.

About the classes, I didn't necessarily had to create a class I just had to execute a function, I don't know why I put them inside the ngClass attribute, I guess I could've done this in a better way by putting them inside the curly brackets inside the html. I suppose that is what you are talking about?

Please show me the comment and I'll fix it :)

[kolkov]

This under code review...

[kolkov]

And you don't need everyday to change path to library if you run build and watch lib and then run start to build application... All refreshes automatically if you change library source code.

[kolkov]

And try to use Goland + AngularJS plugin or Webstorm EAP to developing. It is very convenient and intelligent IDEs.

https://blog.jetbrains.com/go/2020/02/06/welcome-to-the-goland-2020-1-eap/

https://confluence.jetbrains.com/display/WI/WebStorm+EAP

[Simbaclaws]

odd, I can't see the code review messages, are they still pending to be added to the code review? I literally don't have them.

Github shows: No reviews

EDIT: I think you'll have to do this in order for me to see them: https://github.com/sindresorhus/refined-github/issues/2063

EDIT2: Tomorrow I'll make a new commit. But I do think that we'll need the timeout in order to have the animation work when pressing random images.

Since otherwise these classes are added simultaneously, which doesn't toggle the from-right or from-left class towards the active class. The active class needs to be added after the from-left and from-right class which should then since it was first translateX(-100%) or translateX(100%) translate to: translateX(0).

I suppose we can add a {{addActive(image.index, curimage)}} inside the template instead of in the class. And use the method for generating all of the classes. Although it would make sense to show the classes in the template IMO... Why put this in the logic? It would be a lot easier to see these classes when viewing the html, since that would be the place you'd expect to see them I guess.

Just the methods that are being called should be inside of it's own curly brackets: {{}} instead of within the classes code IMO.

EDIT 3: About the IDE's, you're right, I should start coding in a IDE instead of a code editor I guess. I'm thinking about buying webstorm.

[kolkov]

Thanks!

[kolkov]

At the moment, this commit does not solve the problem with delayed frame animation.

[kolkov]

Please check the master branch for changes. By the way, it is possible that approach c will be successful without using Angular animations: https://github.com/kolkov/ngx-gallery/blob/aa864a0ecd4f4d74ffffc0f290058412da07c852/projects/gallery/src/lib/ngx-gallery-image/ngx-gallery-image.component.ts#L225

[Simbaclaws]

This indeed does not solve the delayed frame animation problem, what it does solve is that images won't dissapear when clicking on the next thumbnail or next arrow repeatedly. A bug I found when trying to find issues. EDIT3: (Which apparently only exists in my repository, your approach doesn't have this bug)

I haven't found a way to fix the delayed animation problem just yet but I have fixed a other bug that I saw was appearing.

I will check for the conflicts and fix them.

What do you mean with approach c?

EDIT: Ah, I see you changed the master branch, I'll have a look and see whether this is a better approach.

EDIT2: I checked out the master branch, your approach seems a lot better then mine. I guess this is solved then? Should I close this pull request?

EDIT4:

Perhaps it's not a bad idea to add my latest changes with the isAnimating boolean. https://github.com/kolkov/ngx-gallery/pull/24/commits/32fa38bf0c334dcf2fe338405d74119ba1273bec#diff-391906b00a686cfb5efc80c7b07aefbaR181-R201

This would make it impossible to quickly switch between thumbnails while the animation is still doing its thing, causing no white delay to show up either. (not sure if that is what you want?)

When will you push these changes from the master branch to the npm repository?

[kolkov]

I will check for the conflicts and fix them.

What do you mean with approach c?

EDIT: Ah, I see you changed the master branch, I'll have a look and see whether this is a better approach.

EDIT2: I checked out the master branch, your approach seems a lot better then mine. I guess this is solved then? Should I close this pull request?

EDIT4:

Perhaps it's not a bad idea to add my latest changes with the isAnimating boolean. 32fa38b#diff-391906b00a686cfb5efc80c7b07aefbaR181-R201

This would make it impossible to quickly switch between thumbnails while the animation is still doing its thing, causing no white delay to show up either. (not sure if that is what you want?)

When will you push these changes from the master branch to the npm repository?

Please check all possible problems that we can get by radically changing the approach. One head is good, and two is better, as they say. The only thing I see is that we are depriving people of some kind of customization through the use of redefinition in and other possible animations. What are your thoughts on this approach? https://blog.thoughtram.io/angular/2017/07/26/a-web-animations-deep-dive-with-angular.html

[kolkov]

https://ngx-gallery.kolkov.ru/ live preview with latest changes. Yes, if you switch very quickly, sometimes problems happen.

EDIT: Another click on the thumbnails incorrectly selects the active frame. https://github.com/kolkov/ngx-gallery/blob/6e01c959ad724dcd6feedc0223691a79f2e73149/projects/gallery/src/lib/ngx-gallery-image/ngx-gallery-image.component.ts#L52

[Simbaclaws]

is https://ngx-gallery.kolkov.ru live? Because I can see a difference between the master branch and this live preview. The live preview looks way worse then the master branch when doing git clone.

So what you are saying is that the approach you took for animating the next and previous image is something we'd need to do for all different sort of animations?

This is the reason why I wanted to do it with css classes, if you look at my repository you can see that all different types of animations work. Perhaps we might need to do something similar but with angular animations instead. I'll have a look at what I can do to the master branch.

For the problem when clicking multiple times I say maybe use the isAnimating boolean perhaps? Or would this be limiting to the user? This way the same thumbnail or a different thumbnail can't be clicked until the animation finishes.

[kolkov]

I fixed all known issues, and was update live example.

[Simbaclaws]

Thank you, you're awesome.

I see that the master branch doesn't have a fix for the other animations. I made a fix for that.

This happened at the same time you were working on your branch.

Rotate left, rotate right, zoom, fade and slide right and slide left work in my commit.

You can use animation states to select a different animation basically. And then use void again for animating.

[Simbaclaws]

Odd, the conflicts don't seem to resolve....

Checkout my latest master branch please.

[kolkov]

I think we need to add transition duration and isAnimation options to config. And we need to extract transitions to external file in future.

[kolkov]

I think we need to close this PR and start another clean PR with latest changes only.

[Simbaclaws]

Agreed