Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[feat] technology filter functionality #85
[feat] technology filter functionality #85
Changes from 3 commits
93f6f67
9c1dca3
6468f17
5f7bd30
0116ffa
eb902a1
37eee59
70a37f9
2da013d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 142 in api/maps/AddMarkers.tsx
Check warning on line 142 in api/maps/AddMarkers.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure how these are used to filter projects. Perhaps we could setup the logic for filtering by all criteria (built in a way that would allow us to add/remove filters). For each project, we'd need to check that each selected feature is satisfied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh I see, that's a good idea! I made the filters under the impression that they wouldn't need to be changed, but I agree that making the logic more flexible would be better in the long run.
The way filtering works currently is that I have a pre-defined object of possible filters (
selectedFilters
) that the projects can filter through within thehandleFilterButtonClick
method.selectedFilters
gets updated every time a new filter is applied or un-applied. Is there anything specifically you would want me clarify?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to make sure all filters were being used. It seems that the current implementation hardcodes filtering according to the
technology
criteria. For flexibility purposes, we could iterate over all attributes of theselectedFilters
object in a way that is agnostic to what the filters actually are.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh I see, I think I'm doing that? The
technology
variable in line 80 is just a list that holds a bunch of strings (or is empty). Then line 82 filters all the projects by checking if itsrenewable_energy_technology
attribute is within thistechnology
list, without caring about specific values. Is this what you mean about not hardcoding the attributes?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, given that this PR is only for the technology filter, your implementation suffices. I was just curious if there was a way to iterate over the
selectedFilters
object so we could easily implementstatus
,projectSize
, andlocation
filters as well. However doing so in a way where each filter is it's own function/reducer that, when applied to a given row, can immediately determine whether it belongs in the output. Does that make sense? It might not be necessary for this implementation, but it's one possible approach.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh I see now, you're referring to the general filters. I think we can definitely do that and it makes sense to do so- this probably will involve some mapping of the filters to the project attributes in question. And then the method can just do an agnostic loop over that mapping to filter the projects. Thanks Ronnie! I'll adjust my PR for this.