[feat] technology filter functionality #85
Conversation
ethan-tam33
changed the title
There was a problem hiding this comment.
Make sure you keep all the stuff in the AddMarkers file for now please. Other than that, you can squash and merge. Please make sure to squash and merge, you can do this by clicking on the arrow button next to merge and selecting the "Squash and Merge" option. I think you have just been merging commits without squashing, resulting in all your commits being pushed separately to main.
There was a problem hiding this comment.
I left a few mostly conceptual comments regarding the filter process. Looking good so far! Props for tackling a fairly complex feature! @ethan-tam33 @itsliterallymonique
| // show projects based on selected filters | ||
| const handleFilterButtonClick = () => { | ||
| /* eslint-disable @typescript-eslint/no-unused-vars */ | ||
| const { status, technology, projectSize, location } = selectedFilters; |
There was a problem hiding this comment.
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.
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 the handleFilterButtonClick 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.
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 the selectedFilters object in a way that is agnostic to what the filters actually are.
There was a problem hiding this comment.
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 its renewable_energy_technology attribute is within this technology list, without caring about specific values. Is this what you mean about not hardcoding the attributes?
There was a problem hiding this comment.
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 implement status, projectSize, and location 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.
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.
| export const FilterIconStyles = styled.div` | ||
| display: flex; | ||
| align-items: center; | ||
| flex-direction: row; | ||
| `; |
There was a problem hiding this comment.
Nit: Any chance the FilterIconStyles style could extend a button instead of a div? It seems to be using the onClick property in the TechnologyDropdown which is an attribute that typically belongs to buttons.
There was a problem hiding this comment.
Hmm previously only ButtonWithIconStyles had the onClick property, but I had to manually add it for each of the sub-components in this sprint because of the temporary clearFilters method on the exit button. I don't think this change will be permanent, but I do agree that we should look into making ButtonWithIconStyles extend a button instead of a div.
| let projectsToSearch: Project[] = []; | ||
|
|
||
| if (filteredProjectsFromDropdowns.length > 0) { | ||
| projectsToSearch = filteredProjectsFromDropdowns; | ||
| } else { | ||
| projectsToSearch = projects; | ||
| } | ||
|
|
||
| const searchedProjects: Project[] = | ||
| projectsToSearch?.filter(project => |
There was a problem hiding this comment.
Can filters be applied after a search is made? In that case, would it make sense to search then filter?
I wonder if there's a way to treat "search" similar to a filter criteria. Each filter would then be applied sequentially in the order they're selected.
There was a problem hiding this comment.
The way that I've implemented it allows for filtering after a search is made! I think it does make sense to search then filter? For example, if someone wanted to checkout a project that has a common word "north", "south", etc. and then wanted to filter by dropdowns. This is probably a niche, rare use case but the way that the filtering is implemented allows for search then filter and filter then search to produce the same outputs in the project modal.
Do you mind explaining your idea of sequentially applying filters? I'm not quite sure I understand how the projects would be rendered differently.
There was a problem hiding this comment.
Ok it seems that the implementation already works as intended then. I was just curious about the ordering that filters are applied, because it could affect performance.
For example, if you wish to search for "north" and filter by "hydroelectric," it might make sense to filter first then search, because depending on the search implementation, I imagine search would be an asymptotically slower operation (??), and thus better to perform on a smaller subset of projects.
The more I think about it, this is entirely dependent on search logic and how project data is stored, which I don't have much context on. Just something to consider!
There was a problem hiding this comment.
I agree with Ronnie. We should filter --> search. Every time the user types a letter to search we are searching based on what the user has typed so far, so I think it would be better to search on a smaller subset. @ethan-tam33 can you adjust your sprint to filter then search?
There was a problem hiding this comment.
Hmm are you both suggesting that we prevent any searching until the user clicks on at least one filter?
Regarding the time complexity of searching, I believe the way that we have set it up has filtering for search and dropdown filters to have the same complexity. We're iterating over each project and checking if it includes the search term or is included in the dropdown filters.
There was a problem hiding this comment.
I see that search isn't very complex in this case. Although I think the reason I bring this up is based off the main useEffect, where projectsToSearch selects all filtered projects, then the search term is applied to this list. Suppose I have filtered by hydroelectric then searched for "north". I'd now have a list of projects satisfying these criteria. If I additionally applied the operational filter, ideally I'd take the list of already filtered projects, and apply the additional filter there. However, it seems that filters are applied first, separately from search, then the search term is applied after. Do you see what I mean? I'm not sure if that makes sense or whether it's actually needed, but something I noticed.
There was a problem hiding this comment.
Ohh ok I see what you are saying! I agree that we should do this for filters part of the Status filter. However, I don't think we can do this for a flow like hydroelectric -> search for north -> land-based wind (aka another technology filter). We would need to filter all projects based on the technology filters and those wouldn't exist in the modal prior to selecting land-based wind. Perhaps this is a task that Neha's recent PR can reflect then?
There was a problem hiding this comment.
For now, just squash and merge your sprint so that I can check everything works smoothly for this week's sprints. We can sort this out another time.
|
@ronniebeggs Thanks for the comments! I'll squash and merge once we resolve our conversations here. |
What's new in this PR
Description
clearFiltersfunction for future useScreenshots
Screen.Recording.2024-11-24.at.12.01.16.AM.mov
How to review
MapViewScreen.tsxfor all filtering logicNext steps
clearFiltersfunction in a new button or existing UI componentRelevant links
Online sources
Related PRs
CC: @itsliterallymonique