-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix/loading issue business settings #14
Conversation
|
@@ -10,9 +10,10 @@ import { NGXLogger } from 'ngx-logger'; | |||
import { SnackService } from '../../core/service/snack.service'; | |||
import { ShopService } from '../../core/service/shop.service'; | |||
import { IShopData } from '../../core/model/business'; | |||
import { ChangeDetectorRef } from '@angular/core'; |
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 can consolidate this w/ the existing @angular/core import.
reader.readAsDataURL(event.target.files[0]); | ||
this.image = event.target.files[0]; | ||
this.isLoadingPreview = true; | ||
onFileChange(event: Event): void { |
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 you adjust the alignment in this function?
|
||
const response = await this.shopServices.registerShop(datas); | ||
reader.onerror = () => { | ||
console.error("Failed to read file!"); |
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.
We want to move away from client-side console logging, especially in prod. Can you change this and other console instances to a logger instead? You can reference logger instances in the existing code.
this.registerShopForm.markAllAsTouched(); | ||
this.logger.warn('Invalid data logged'); | ||
this.logger.info(this.registerShopForm.value); | ||
} catch (error) { |
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.
Can you align the catch
and finally
block?
} | ||
} else { |
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.
Can you align the else
block?
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 have my approval granted you tested the new changes in the sandbox + PR comments have been addressed. Comments are mostly regarding minor adjustments with the exception of console logging. Other than that, the main mod looks good; great work, @DarinHajou!
Fixed loading issue with the business registration image. The image use to load slow or stuck at infinite loading. With these changes the image loads instantaneously.
NB! Merge the changes on this branch on need by need basis.