Skip to content
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

#791 #1027

Merged
merged 24 commits into from
Apr 19, 2022
Merged

#791 #1027

merged 24 commits into from
Apr 19, 2022

Conversation

LeraKornienko
Copy link
Contributor

No description provided.

create without classes
paginator
pagg
pgg
search + paging
test
Classes
@@ -15,6 +15,8 @@ export class PaginatorComponent implements OnInit, OnChanges {

@Input() currentPage: PaginationElement;
@Input() totalEntities: number;
@Input() totalEntitiesDir: number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the same input ad it was before totalEntities
there is no sense in the new one

Comment on lines 49 to 53
export enum createDirectionSteps {
'direction',
'department',
'class'
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't think we should keep this enum in provider.ts. move it to admin

createDirection(direction: Direction): Observable<Direction> {
return this.http.post<Direction>('/api/v1/Direction/Create', direction);
}
createDepartment(department: Department): Observable<object> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add typo Observable

export class UpdateDirection {
static readonly type = '[admin] update Direction';
constructor(public payload) { }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add typo for payload

Comment on lines 18 to 19
iClass: IClass[];
classes: IClass[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we have two fields for classes? I think you can use just one of them and patch it

templateUrl: './directions.component.html',
styleUrls: ['./directions.component.scss']
})
export class DirectionsComponent implements OnInit {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add OnDestroy

Comment on lines 54 to 57
this.searchValue.valueChanges
.pipe(takeUntil(this.destroy$))
.subscribe((val: string) => {
this.searchedText = val;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spacing

this.store.dispatch([new FilterClear(), new GetFilteredDirections(), ]);

this.searchValue.valueChanges
.pipe(takeUntil(this.destroy$))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add additional operators such as debounce time and startWith(''), distinctUntilChanged(),
check it for the search-filed component/city filter, we have the same there

Comment on lines 63 to 65
this.searchQuery$
.pipe(takeUntil(this.destroy$))
.subscribe((text: string) => this.searchValue.setValue(text, {emitEvent: false}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here I believe

}

onSearch(): void {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove empty line

@@ -1,7 +1,8 @@
export enum NoResultsTitle {
noApplication = 'Заявок поки немає',
noResultWorkshops = 'За результатами нічого не знайдено',
noParentWorkshops = 'Тут буде відображатися інформація про гуртки, секцію або школу для навчання. Ще жодної заяви на гурток не подано.',
noParentWorkshops = 'Тут буде відображатися інформація про гуртки, секцію фбо школу для навчання. Ще жодної заяви на гурток не подано.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mistake

Comment on lines 61 to 62
private getTotalPageAmount(): number {
return Math.ceil(this.totalEntities / Constants.ITEMS_PER_PAGE);
return Math.ceil(this.totalEntities / Constants.ITEMS_PER_PAGE) || Math.ceil(this.totalEntitiesDir / Constants.ITEMS_PER_PAGE_DIR);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just as an idea, maybe for getTotalPageAmount method will be good to receive from parent component flag like isDirections and then check and if yes use Constants.ITEMS_PER_PAGE_DIR

Comment on lines 29 to 30
static readonly ITEMS_PER_PAGE_DIR =10;
static readonly ITEMS_PER_PAGE = 2 * Math.floor(window.innerWidth / (332));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think is it make sense to use the same approach as with ITEMS_PER_PAGE constant? I mean "2 * Math.floor(window.innerWidth / (332))"

@@ -1,5 +1,7 @@
export enum ModalConfirmationType {
delete = 'delete',

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove empty line

this.id = id;
this.title = info.title;
this.description = info.title;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty line

throwError(payload);
dispatch(new ShowMessageBar({ message: 'На жаль виникла помилка', type: 'error' }));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add empty line between methods

Comment on lines 226 to 228
return patchState({ selectedDirection: direction, isLoading: false });
}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tap((direction: Direction) => {
return patchState({ selectedDirection: direction, isLoading: false });
}));
tap((direction: Direction) => patchState({ selectedDirection: direction, isLoading: false })));

@@ -11,9 +11,10 @@ import { SetDirections } from '../../store/filter.actions';
})
export class CategoryCardComponent implements OnInit {
@Input() workshopsCount: number;
@Input() isEditMode: boolean;
@Input() isEditMode: true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true is not a typo of data:) if you want to set a default value then it should be @input() isEditMode = true;
if you want to say the data typo is boolean then you should remain the previous version

@@ -22,6 +23,10 @@ export class CategoryCardComponent implements OnInit {
ngOnInit(): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can delete it as well

dispatch(new ShowMessageBar({ message: 'Дитина успішно відредагована', type: 'success' }));
this.router.navigate(['/admin-tools/platform/about']);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add empty line

this.router.navigate(['/admin-tools/platform/directions']);
this.getFilteredDirections;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add empty line

});
dialogRef.afterClosed().subscribe((result: boolean) => {
if (result) {
const department = result && new Department(this.departmentFormGroup.value, this.direction.id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don think that you should check for result one more time, because you handled it earlier

Comment on lines 62 to 66
debounceTime(1000),
distinctUntilChanged(),
startWith(''),
takeUntil(this.destroy$)
).subscribe(() => this.store.dispatch(new GetFilteredDirections()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't think you need startWith(''),

});

this.searchQuery$
.pipe(takeUntil(this.destroy$))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add operators

@litvinets litvinets merged commit 0607342 into develop Apr 19, 2022
@litvinets litvinets deleted the #791 branch April 19, 2022 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants