Skip to content

feat: integrate Unused-Guid-Search #3319

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sogladev
Copy link
Member

This integrates https://github.com/Kitzunu/Unused-Guid-Search as a tab

UI Screenshot

keira3_unused

I copied the minimal UI, and the feature supports the same tables

It's still missing tests and translations, but I would prefer some feedback before, as I'm not too familiar with Angular architecture

sogladev and others added 2 commits May 27, 2025 23:43
@Helias
Copy link
Member

Helias commented Jun 1, 2025

I made some minor refactoring.

We usually don't use async/await but we work with rxjs, I see that you are using firstValueFrom to avoid rxjs and use async,await. If for you it's better to work with this, you can keep it as it is.
Moreover, I see that you are using ngModel, we are using more the FormGroup and FormControls in Keira3 so I would use them, but if you want to keep everything simple as it is to speed up the development I don't mind, especially because the forms will be converted soon in "SignalForms" in Angular, so sooner or later we'll refactor them.

About other variables, I converted them into signals, and removed the cdr change detection trigger, it is not necessary if you use signals.
Signals are a specific Angular features wich improve the application performances, avoid memory leak, set memorization automatically and many other features, so if possible I would still use them.

For the rest, the job seems a good start point!

@sogladev
Copy link
Member Author

sogladev commented Jun 2, 2025

@Helias, Thanks for the feedback and the improvements. I've refactored as needed to match the rest of the project, and I'lll look into setting up tests sometime in the future.

startIndex: [1, [Validators.required, Validators.min(1), Validators.max(this.MAX_INT_UNSIGNED_VALUE)]],
amount: [10, [Validators.required, Validators.min(1), Validators.max(this.MAX_INT_UNSIGNED_VALUE)]],
consecutive: [false, [Validators.required]],
});
Copy link
Member

Choose a reason for hiding this comment

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

you could use protected readonly form: FormGroup<FormGroupINTERFACE>, so let's define an interface with the form group content.
Moreover I would avoid to use the formBuilder, use new FormGroup<FormGroupINTERFACE>({ .... })


protected results: string[] = [];
protected loading = signal(false);
protected error = signal('');
Copy link
Member

Choose a reason for hiding this comment

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

I forgot to set the signals as readonly

protected loading = signal(false);
protected error = signal('');

protected async onSearch(): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

the async should not be necessary anymore

selector: 'keira-features-unused-guid-search',
templateUrl: './unused-guid-search.component.html',
styleUrl: './unused-guid-search.component.scss',
imports: [FormsModule, ReactiveFormsModule, TranslateModule],
Copy link
Member

Choose a reason for hiding this comment

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

you are importing TranslateModule but you are not using any translations, in the future you could add them before merging this PR

})
export class UnusedGuidSearchComponent {
private readonly mysql = inject(MysqlQueryService);
private readonly fb = inject(FormBuilder);
Copy link
Member

Choose a reason for hiding this comment

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

the FormBuilder is not necessary

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.

3 participants