-
Notifications
You must be signed in to change notification settings - Fork 172
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
Support Videos #257
base: master
Are you sure you want to change the base?
Support Videos #257
Conversation
- Tested with .mp4 on Chrome - Done on fast. So there could be things like : - Duplication on CSS classes usage - Some scenarios may have not been handled - Many enhancments could be done
this.loading = false; | ||
this.startAutoPlay(); | ||
this.changeDetectorRef.markForCheck(); | ||
} else if (this.type == 'video') { | ||
|
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.
I feel that videos could require something different than what is inside the above block. I kept this condition, with an empty block, it to highlight that.
So, this condition else if (this.type == 'video')
should either be removed along with its empty block. Or, the condition || this.type == 'video'
should be removed and the empty block should be filled of what is required.
I created a fast NPM package at https://www.npmjs.com/package/ngx-image-video-gallery to be used in the mean time. |
src/ngx-gallery.component.ts
Outdated
@@ -268,9 +268,14 @@ export class NgxGalleryComponent implements OnInit, DoCheck, AfterViewInit { | |||
} | |||
|
|||
private setImages(): void { | |||
const _this:NgxGalleryComponent = this; |
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.
Could be avoided with fat arrow function (if it doesn't have to be compatible with angular aot compilation)
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.
You are write. I replaced it with arrow function.
return fileSource.substr(5, Math.min(fileSource.indexOf(';'), fileSource.indexOf('/')) - 5); | ||
} | ||
let fileExtension = fileSource.split('.').pop().toLowerCase(); | ||
if (!fileExtension |
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.
Maybe using a indexOf with an array of file type could be better?
Moreover, using mime type could be a better solution (look for mime/type supported by current browsers) & I don't think that using substr is useful there.
fileSource.indexOf("video/mp4")
as example is enough if fileSource is a dataUrl.
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.
Yes indexOf
is better for most of the cases. But substr
I think that here it will make this piece of code compatible with any content type. If this component will support other types later. This line can stay the as-is.
However, there was unnecessary usage of substr
inside the next else if
. I deleted it in the second commit.
src/ngx-gallery-image.component.ts
Outdated
<div class="ngx-gallery-icons-wrapper"> | ||
<ngx-gallery-action *ngFor="let action of actions" [icon]="action.icon" [disabled]="action.disabled" [titleText]="action.titleText" (onClick)="action.onClick($event, image.index)"></ngx-gallery-action> | ||
<div *ngFor="let image of getImages(); let i = index;"> | ||
<div *ngIf="image.type == 'image'" class="ngx-gallery-image" [ngClass]="{ 'ngx-gallery-active': selectedIndex == image.index, 'ngx-gallery-inactive-left': selectedIndex > image.index, 'ngx-gallery-inactive-right': selectedIndex < image.index, 'ngx-gallery-clickable': clickable }" |
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.
Use ===
instead of ==
, maybe using a else with a ng-template
is better ?
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.
I updated the code to have ===
.
However, I do not like the Angular style of if else
especially when there is if, else-if and else.
Hello @myerffoeg, |
Hi @myerffoeg, |
@myerffoeg are you considering merging @Muhammad-Altabba 's updates to the library ? please consider his request |
@myerffoeg Please merge this PR and also, update the package for Angular 8 support. |
@codemx @yashwp @Muhammad-Altabba I do not have the write access right to do that, I have just done a code review to accelerate the process for the repository author. If you want to accelerate the process you can : review the code, suggest improvements or contact the author @lukasz-galka . |
Thanks @myerffoeg, Hello @lukasz-galka, |
Hello @lukasz-galka |
Hi @lukasz-galka , can we kindly proceed with this request? |
Hi guys, I will take a look on it on Monday |
Hi @lukasz-galka we are still waiting for this.. can you kindly proceed? |
@lukasz-galka @codemx Any luck on this feature? |
@steveacalabro Not sure why @lukasz-galka is not looked at this as he promised around 6 weeks ago. he is the owner of this and we all are waiting for his action. @Muhammad-Altabba is there any alternatives ? |
Is this getting merged anytime soon? @lukasz-galka @steveacalabro |
+1 |
I will try to implement in my Angular 8++ repo. |
@lukasz-galka will this request be merged soon? |
@kolkov Thx for creating an angular 8 version. Wondering, if you will merge these changes to your repo soon? |
definetly that would be a very nice addition |
This merged. Try latest version please. |
This should be merged, i'm already using, but i believe i found a bug, if i have more than one video when i click to change to the next one, it changes to the same one, but if i place an image between the videos it works fine. |
Hello guys, |
Tested with .mp4 on Chrome
Done on fast. So there could be things like :
- Duplication on CSS classes usage
- Some scenarios may have not been handled
- Many enhancements could be done
Related to Enhancement: (add videos in the gallery) #149