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

Refactor filters into its own service #249

Closed
7 tasks done
Arif-Khalid opened this issue Feb 8, 2024 · 4 comments
Closed
7 tasks done

Refactor filters into its own service #249

Arif-Khalid opened this issue Feb 8, 2024 · 4 comments
Labels
aspect-CodeQuality aspect-Scalability category.Chore a task that needs to be done difficulty.Moderate can be done after you have done a few easy issues first s.ToDiscuss

Comments

@Arif-Khalid
Copy link
Contributor

Arif-Khalid commented Feb 8, 2024

Is your feature request related to a problem? Please describe.
It is frustrating that the filters are declared in multiple components and observables are prop drilled into other components in order to make things work. As a developer working on the filters, it is very difficult to follow.
For example, labelFilter BehaviourSubject is delared in filter-bar, and prop drilled into label-filter-bar.
label-filter-bar is then the component that initializes the labels, calling .next on the behaviour subject from filter-bar.

Additionally, certain filters are referenced objects such as labels being an array and hidden labels being a set. We also use a DEFAULT_DROPDOWN_FILTER variable as the base for when filters are reset.
Because of these referenced filters, an easy mistake for developers would be these objects' mutation by components, resulting in a permanent mutation of the base DEFAULT_DROPDOWN_FILTER.

This follows from the solution for #205, intended to be worked on after that PR, #226, is merged in.

Describe the solution you'd like
A new service filters.service.ts that contains the current repo filters stored as observables and subscribed to the relevant github services such as labelService subscription.
Filters are encapsulated within this component.
Components use the observable's .next and .subscribe functions to access and modify filters

Describe alternatives you've considered
We can leave the filters as is and risk errors down the line due to the issues mentioned.
We can use a context provider to lift the state out instead of using a filters service.

  • Refactor selected label filter
  • Refactor hidden label filter
  • Refactor type filter
  • Refactor status filter
  • Refactor sort filter
  • Refactor milestone filter
  • Refactor title filter
@Arif-Khalid Arif-Khalid added category.Feature a new feature aspect-CodeQuality aspect-Scalability difficulty.Moderate can be done after you have done a few easy issues first s.ToDiscuss category.Chore a task that needs to be done and removed category.Feature a new feature labels Feb 8, 2024
@Arif-Khalid
Copy link
Contributor Author

@CATcher-org/2223s2 @CATcher-org/2324s2

@nknguyenhc
Copy link
Contributor

Agree. I think this one should be done to set the service to be used as solution for #205 (currently #226 is attempting to solve).

What do yall think? @gycgabriel @vigneshsankariyer1234567890

@vigneshsankariyer1234567890
Copy link
Contributor

I think it makes a lot of sense to have filters as its own service. That way, we have a neat abstraction to make changes to the filters when needed and access the data within the filters themselves

@Arif-Khalid
Copy link
Contributor Author

All filters have been refactored

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aspect-CodeQuality aspect-Scalability category.Chore a task that needs to be done difficulty.Moderate can be done after you have done a few easy issues first s.ToDiscuss
Projects
None yet
Development

No branches or pull requests

3 participants