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

taxonomy-editor create term changes #2

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

deviigopinath
Copy link

No description provided.

Copy link

@HarishGangula HarishGangula left a comment

Choose a reason for hiding this comment

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

Following changes need to be done to review the PR

  1. The Coverage folder needs to be in gitignore
  2. Dist folder also needs to be in gitignore

Delete all the tokens added in the PR

README.md Outdated
channelId: string,
authToken: string,
isApprovalRequired: boolen // set default to false,

Choose a reason for hiding this comment

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

boolean spelling mistake

@santhosh-tarento
Copy link

@HarishGangula Can you please look into this PR, if any comments missed

Copy link

@HarishGangula HarishGangula left a comment

Choose a reason for hiding this comment

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

  1. Remove all the commented and unused code
  2. Update or add the test cases for the required code

</button>
</div>
</div>
<!-- <button mat-raised-button color="primary" (click)="publishFramwork()">Publish</button> -->

Choose a reason for hiding this comment

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

remove commented tags

app_strings: any = labels;
constructor(private frameworkService: FrameworkService) { }

ngOnInit() {

Choose a reason for hiding this comment

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

remove unused methods

ngOnInit() {
}

SendForApproval(){

Choose a reason for hiding this comment

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

keep code style consistent follow camelcase for method names

import { ActionBarComponent } from './action-bar.component';
import { HttpClientModule } from '@angular/common/http';

describe('ActionBarComponent', () => {

Choose a reason for hiding this comment

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

No test case has proper assertions example: closeActionBar method emits a event but that is not asserted

</div>
<div>
<button mat-raised-button color="primary" (click)="SendForApproval()">
{{actionType? getApproveLevelText(configType) :'Send for Approval'}}

Choose a reason for hiding this comment

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

can we move this logic to ts file

@@ -0,0 +1,15 @@
import { Component, OnInit } from '@angular/core';

Choose a reason for hiding this comment

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

Delete this component if not used

console.log('Service...',res)
this.frameworkCategories = res.result.framework.categories
})
// this.categoriesRepresentations = categoryRepresentationsV1

Choose a reason for hiding this comment

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

Delete the commented code

import { HttpClientModule } from '@angular/common/http';
import { MatDialogModule } from '@angular/material/dialog';

xdescribe('ConfigFrameworkComponent', () => {

Choose a reason for hiding this comment

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

Add the test cases

<!-- <p>{{data.children.approvalStatus}}, {{data.children.associationProperties?.approvalStatus}}</p> -->
<!-- *ngIf="!data.isViewOnly && data.children.approvalStatus === 'Draft' && isApprovalRequired" -->
<div class="term-card-checkbox">
<mat-checkbox color="primary" *ngIf="data.children.approvalStatus === 'Draft' && isApprovalRequired && !approvalList.length ||

Choose a reason for hiding this comment

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

Can we move this logic to method in ts and call that method

@@ -0,0 +1,350 @@
import { Component, ElementRef, Input, OnChanges, OnInit, Output, SimpleChanges, ViewChild, EventEmitter, OnDestroy } from '@angular/core';

Choose a reason for hiding this comment

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

Delete the commented code and unused imports

@santhosh-tarento
Copy link

santhosh-tarento commented Mar 13, 2024

@HarishGangula Changes are pushed, Could you please look into this PR.

Copy link

@HarishGangula HarishGangula left a comment

Choose a reason for hiding this comment

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

Still unit test cases are not written for basic flows in the components
Which need to be addressed

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