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

Refactor sorting #261

Merged
merged 20 commits into from
Feb 28, 2024
Merged

Refactor sorting #261

merged 20 commits into from
Feb 28, 2024

Conversation

Arif-Khalid
Copy link
Contributor

@Arif-Khalid Arif-Khalid commented Feb 22, 2024 •
edited
Loading

Summary:

Fixes part of #249

Type of change:

  • 🎨 Code Refactoring

Changes Made:

  • Refactored sorting behaviour of IssueDataTable into dropdownFilters
    • Removed reference of MatSort in IssueDataTable
    • Replaced with matsort Sort object stored in dropdown filters
    • Removed unused code

Proposed Commit Message:

MatSort is passed by reference into IssueDataTables,
lifted up from filter-bar component. Changes to 
MatSort are detected and handled in IssueDataTables.

MatSort reference in IssueDataTables performs the 
responsibility of a dropdown filter outside of the 
filters service.
This violates good programming practices.

Let's refactor the sort filter into the filters service.

Checklist:
  • I have tested my changes thoroughly.
  • I have created tests for any new code files created in this PR or provided a link to a issue/PR that addresses this.
  • I have added or modified code comments to improve code readability where necessary.
  • I have updated the project's documentation as necessary.

@Arif-Khalid Arif-Khalid self-assigned this Feb 22, 2024
@Arif-Khalid Arif-Khalid changed the title Refactor sorting behaviour Refactor sorting Feb 22, 2024
nknguyenhc
nknguyenhc requested changes Feb 23, 2024
View reviewed changes
Copy link
Contributor

@nknguyenhc nknguyenhc left a comment

Choose a reason for hiding this comment

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

image

Filter on labels do not work. Any other filters similarly do not work.

src/app/shared/issue-tables/dropdownfilter.ts
Comment on lines -26 to 32
export function applyDropdownFilter(dropdownFilter: DropdownFilter): (a: Issue) => boolean {
return (issue) => {
export function applyDropdownFilter(dropdownFilter: DropdownFilter, data: Issue[]): Issue[] {
const filteredData: Issue[] = data.filter((issue) => {
let ret = true;

if (dropdownFilter.status === 'open') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean filteredData is never used in the return statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent catch. I must have had uncommited local changes. The error has been fixed and all filters should be working now.

nknguyenhc
nknguyenhc previously approved these changes Feb 24, 2024
View reviewed changes
Copy link
Contributor

@nknguyenhc nknguyenhc left a comment

Choose a reason for hiding this comment

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

LGTM!

vigneshsankariyer1234567890
vigneshsankariyer1234567890 requested changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall, slick PR and nice changes to integrate changes from filters! Just a few comments regarding SRP! 😄

src/app/shared/issue-tables/dropdownfilter.ts Outdated Show resolved Hide resolved
src/app/shared/issue-tables/issue-sorter.ts Outdated Show resolved Hide resolved
src/app/shared/issue-tables/IssuesDataTable.ts Show resolved Hide resolved
vigneshsankariyer1234567890
vigneshsankariyer1234567890 approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely done, LGTM! 🚀

@Arif-Khalid Arif-Khalid merged commit ad94663 into CATcher-org:main Feb 28, 2024
3 checks passed
@NereusWB922 NereusWB922 mentioned this pull request Mar 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vigneshsankariyer1234567890 vigneshsankariyer1234567890 vigneshsankariyer1234567890 approved these changes

@nknguyenhc nknguyenhc nknguyenhc left review comments

@gycgabriel gycgabriel Awaiting requested review from gycgabriel

@NereusWB922 NereusWB922 Awaiting requested review from NereusWB922

@MadLamprey MadLamprey Awaiting requested review from MadLamprey

Assignees

@Arif-Khalid Arif-Khalid

Labels
aspect-CodeQuality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Arif-Khalid @vigneshsankariyer1234567890 @nknguyenhc