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.

Sign up for GitHub

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

Jump to bottom

[MNT-24575] Added dialog to display folder information #4282

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

[MNT-24575] Added dialog to display folder information #4282

wants to merge 9 commits into from

Conversation

swapnil-verma-gl
Copy link
Contributor

JIRA ticket link or changeset's description
https://hyland.atlassian.net/browse/MNT-24575

Added new dialog accessible via context menu to display folder information such as folder location, size, file count etc

MichalKinas
MichalKinas reviewed Dec 6, 2024
View reviewed changes
projects/aca-content/src/lib/dialogs/folder-details/folder-information.component.spec.ts Outdated
modifiedAt: new Date(2024, 2, 2, 22, 22)
};

const getValueFromElement = (id: string) => fixture.debugElement.query(By.css(`[data-automation-id="${id}"]`)).nativeElement.textContent;
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
const getValueFromElement = (id: string) => fixture.debugElement.query(By.css(`[data-automation-id="${id}"]`)).nativeElement.textContent;
const getValueFromElement = (id: string): string => fixture.debugElement.query(By.css(`[data-automation-id="${id}"]`)).nativeElement.textContent;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added return type

projects/aca-content/src/lib/dialogs/folder-details/folder-information.component.ts Outdated
private readonly destroyRef = inject(DestroyRef);

data: Node = inject(DIALOG_COMPONENT_DATA);
name: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need separate variables for name,size, location, created and modified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consolidated all properties into a single FolderInformation type, defined in folder-information.component.ts

projects/aca-content/src/lib/dialogs/folder-details/folder-information.component.ts Outdated
this.nodesService
.getFolderSizeInfo(this.data.id, jobIdEntry.entry.jobId)
.pipe(
expand((result: any) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use proper type instead of any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed instance of any to SizeDetailsEntry

AleksanderSklorz
AleksanderSklorz reviewed Dec 10, 2024
View reviewed changes
projects/aca-content/src/lib/dialogs/folder-details/folder-information.component.ts
this.folderDetails.icon = this.contentService.getNodeIcon(this.data);
this.folderDetails.size = this.translateService.instant('APP.FOLDER_INFO.CALCULATING');

this.nodesService
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add some error handling (for example you can display some error content inside dialog when loading is failed)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@swapnil-verma-gl I guess this one is not addressed because I can't see changes for that one?

@AleksanderSklorz
Copy link
Contributor

@swapnil-verma-gl I'm getting some issue. When I'm opening dialog then many errors are inside console and dialog's look is broken:
image

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

AleksanderSklorz
AleksanderSklorz reviewed Dec 19, 2024
View reviewed changes
projects/aca-content/src/lib/dialogs/folder-details/folder-information.component.ts
.initiateFolderSizeCalculation(this.data.id)
.pipe(
first(),
switchMap((jobIdEntry: JobIdBodyEntry) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to set type in this place because it's inferred by ts:
image

AleksanderSklorz
AleksanderSklorz reviewed Dec 19, 2024
View reviewed changes
projects/aca-content/src/lib/dialogs/folder-details/folder-information.component.ts
? timer(5000).pipe(concatMap(() => this.nodesService.getFolderSizeInfo(this.data.id, jobIdEntry.entry.jobId)))
: EMPTY
),
takeUntilDestroyed(this.destroyRef)
Copy link
Contributor

Choose a reason for hiding this comment

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

takeUntilDestroyed(this.destroyRef) is added for pipe for result of getFolderSizeInfo function. I guess we should add takeUntilDestroyed for pipe for result of initiateFolderSizeCalculation instead as we call subscribe for result of that function?

AleksanderSklorz
AleksanderSklorz reviewed Dec 19, 2024
View reviewed changes
projects/aca-content/src/lib/store/effects/node.effects.spec.ts

describe('folderInformation$', () => {
it('should call folder information dialog', () => {
const node: any = { entry: { isFile: true } };
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to use any. You can use 'as NodeEntry' and correct type will be set

AleksanderSklorz
AleksanderSklorz reviewed Dec 19, 2024
View reviewed changes
projects/aca-content/src/lib/store/effects/node.effects.spec.ts

store.dispatch(new FolderInformationAction(node));

expect(contentService.showFolderInformation).toHaveBeenCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use toHaveBeenCalledWith to test also parameters passed to showFolderInformation function?

AleksanderSklorz
AleksanderSklorz reviewed Dec 19, 2024
View reviewed changes
projects/aca-content/src/lib/dialogs/folder-details/folder-information.component.ts
private readonly destroyRef = inject(DestroyRef);

data: Node = inject(DIALOG_COMPONENT_DATA);
folderDetails: FolderDetails = new FolderDetails();
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to set type at left side when you do inline initialization because ts knows then that type is FolderDetails

@AleksanderSklorz
Copy link
Contributor

AleksanderSklorz commented Dec 19, 2024 •
edited
Loading

@swapnil-verma-gl I see 2 issues, see video:

Screen.Recording.2024-12-19.at.12.51.55.mov
  1. It says that there is 0 bytes but this folder has image inside so should have some bytes.
  2. There is {{count}} placeholder instead of real number

NOTE: After some time, when you wait a bit, it seems working correctly.
NOTE2: Second issue happens also for folders with no files

@AleksanderSklorz
Copy link
Contributor

@swapnil-verma-gl there is some accessibility issue:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AleksanderSklorz AleksanderSklorz AleksanderSklorz left review comments

@nikita-web-ua nikita-web-ua Awaiting requested review from nikita-web-ua

@dominikiwanekhyland dominikiwanekhyland Awaiting requested review from dominikiwanekhyland

@MichalKinas MichalKinas Awaiting requested review from MichalKinas

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@swapnil-verma-gl @AleksanderSklorz @MichalKinas