search

megarohas / lazy-img-loading

Simple Angular project. Lazy Image loading is included.
0 stars 0 forks source link

Serveral comments just for note :) #1

Open lxrave opened 4 years ago

lxrave commented 4 years ago

Hi. There is just some remarks. Nothing serious, just for note for us.

Why did U create own ObserverType instead of standart interface? (https://microsoft.github.io/PowerBI-JavaScript/interfaces/_node_modules_typedoc_node_modules_typescript_lib_lib_dom_d_.intersectionobserver.html) https://github.com/megarohas/lazy-img-loading/blob/7496d739d041967492e3b6adc84da6ba6ba7f571/src/app/lazy-load.directive.ts#L3-L6

Why did U use private access modifier everywhere? https://github.com/megarohas/lazy-img-loading/blob/7496d739d041967492e3b6adc84da6ba6ba7f571/src/app/lazy-load.directive.ts#L3-L6 https://github.com/megarohas/lazy-img-loading/blob/7496d739d041967492e3b6adc84da6ba6ba7f571/src/app/lazy-load.directive.ts#L18 https://github.com/megarohas/lazy-img-loading/blob/7496d739d041967492e3b6adc84da6ba6ba7f571/src/app/lazy-load.directive.ts#L34

Interface OnInit for directive is missing. https://github.com/megarohas/lazy-img-loading/blob/7496d739d041967492e3b6adc84da6ba6ba7f571/src/app/lazy-load.directive.ts#L11

Constructor can be simplified from https://github.com/megarohas/lazy-img-loading/blob/7496d739d041967492e3b6adc84da6ba6ba7f571/src/app/lazy-load.directive.ts#L28-L33 to

  error_img = "https://clck.ru/LaNDr";
  loading_img = "https://clck.ru/LaQ32";

  constructor(protected el: ElementRef) {}

Setting intersection_observer_config can be class property too (just for beauty) https://github.com/megarohas/lazy-img-loading/blob/7496d739d041967492e3b6adc84da6ba6ba7f571/src/app/lazy-load.directive.ts#L36-L38

Class methods order is too random :)

megarohas commented 4 years ago

Hello. Thanks for feedback. Yes, I am agree with these points.

Why did U create own ObserverType instead of standart interface? Yes, it's better to use standard interface.

Why did U use private access modifier everywhere? I have used it because of a habit of using PRIVATE in case of no requirements.

Interface OnInit for directive is missing. Yes, you are right.

Constructor can be simplified.

Got it, thanks.

Setting intersection_observer_config can be class property too (just for beauty) Agree.

Class methods order is too random :)

Agree, sorry for that.

вт, 31 дек. 2019 г. в 01:28, Sergey Komarov notifications@github.com:

Hi. There is just some remarks. Nothing serious, just for note for us.

Why did U create own ObserverType instead of standart interface? ( https://microsoft.github.io/PowerBI-JavaScript/interfaces/_node_modules_typedoc_node_modules_typescript_lib_lib_dom_d_.intersectionobserver.html )

https://github.com/megarohas/lazy-img-loading/blob/7496d739d041967492e3b6adc84da6ba6ba7f571/src/app/lazy-load.directive.ts#L3-L6

Why did U use private access modifier everywhere?

https://github.com/megarohas/lazy-img-loading/blob/7496d739d041967492e3b6adc84da6ba6ba7f571/src/app/lazy-load.directive.ts#L3-L6

https://github.com/megarohas/lazy-img-loading/blob/7496d739d041967492e3b6adc84da6ba6ba7f571/src/app/lazy-load.directive.ts#L18

https://github.com/megarohas/lazy-img-loading/blob/7496d739d041967492e3b6adc84da6ba6ba7f571/src/app/lazy-load.directive.ts#L34

Interface OnInit for directive is missing.

https://github.com/megarohas/lazy-img-loading/blob/7496d739d041967492e3b6adc84da6ba6ba7f571/src/app/lazy-load.directive.ts#L34

Constructor can be simplified from

https://github.com/megarohas/lazy-img-loading/blob/7496d739d041967492e3b6adc84da6ba6ba7f571/src/app/lazy-load.directive.ts#L28-L33 to

error_img = "https://clck.ru/LaNDr"; loading_img = "https://clck.ru/LaQ32";

constructor(protected el: ElementRef) {}

Setting intersection_observer_config can be class property too (just for beauty)

https://github.com/megarohas/lazy-img-loading/blob/7496d739d041967492e3b6adc84da6ba6ba7f571/src/app/lazy-load.directive.ts#L36-L38

Class methods order is too random :)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/megarohas/lazy-img-loading/issues/1?email_source=notifications&email_token=AKZB6WRND7B2JPH7ELMAWWDQ3JDWNA5CNFSM4KBPOWT2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IDMSJ2A, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKZB6WS2TL62QKX24SYERALQ3JDWNANCNFSM4KBPOWTQ .