[feat] Implement Plant Seasonality Filters #17
Conversation
Co-authored-by: Catherine Tan <[email protected]> Co-authored-by: rachaelch3n <[email protected]> Co-authored-by: Catherine Tan <[email protected]>
Co-authored-by: Catherine Tan <[email protected]>
There was a problem hiding this comment.
looking so great, thanks kyle! a couple comments, esp about the growing season filtering logic, but this is a great start!
we'll keep building on this in the next sprint by (1) adding a state filter, (2) making all filters except the state filter multi-select
|
|
||
| const filterPlantList = (plant: Plant) => { | ||
| return ( | ||
| checkGrowingSeason(plant) && |
There was a problem hiding this comment.
i rlly like this structure! it's super clean; great job!
quick note: in the next sprint we'll add a state filter too
components/PlantList.tsx
Outdated
| return ( | ||
| months?.includes(plant.indoors_start) || | ||
| months?.includes(plant.indoors_end) || | ||
| months?.includes(plant.outdoors_start) || | ||
| months?.includes(plant.outdoors_end) |
There was a problem hiding this comment.
i'm wondering if we need to modify the logic here. what if the growing season is Summer, but the indoor time is April-Sept (i.e. it includes all Summer months, but the start and end times are not in the summer months), and outdoors is August-August? I don't think this example would realistically happen, but we also have to account for LATE_MONTH values which aren't included in the growingSeasonToMonth map.
Perhaps we should try to all seasons that the growing season elapses and check if the inputted growing_season is in the list of elapsed seasons.
Perhaps using actual enums might help here? (The type enums that we're using rn are just types and not actual enums oops)
| const us_state = 'TENNESSEE'; | ||
| const filteredPlantList = plantList.filter( | ||
| plant => plant.us_state === us_state, |
There was a problem hiding this comment.
nit: we'll make us_state camelCase in the future
components/PlantList.tsx
Outdated
| // Check if growingSeason matches the plant's growing season | ||
| const checkGrowingSeason = (plant: Plant) => { | ||
| // Automatically returns true if selected growing season is '' | ||
| if (!growingSeason) { |
There was a problem hiding this comment.
let's rename all filter values to e.g. growingSeasonFilterValue
components/PlantList.tsx
Outdated
| const validIndexes = growingSeasonToIndex.get(growingSeason); | ||
|
|
||
| const isInRange = (start: number, end: number, validIndexes: number[]) => { | ||
| // Checks if the start and end months are within the valid range | ||
| if (start <= end) { | ||
| return validIndexes.some(index => index >= start && index <= end); | ||
| } else { | ||
| // Handle wrap-around case (e.g. NOVEMBER to FEBRUARY) | ||
| return validIndexes.some(index => index >= start || index <= end); |
There was a problem hiding this comment.
this is great! just a quick note, we might move checkGrowingSeason and other filter util funcs to /utils in the future since Sashank will also need it for filters on VIew Plants
components/PlantList.tsx
Outdated
| if ( | ||
| indoorsStart && | ||
| (indoorsStart.startsWith('LATE') || indoorsStart.startsWith('EARLY')) | ||
| ) { | ||
| indoorsStart = indoorsStart.substring(5); | ||
| } |
There was a problem hiding this comment.
maybe create a util func that gets the month value, to repeat code repetition with indoorsStart, indoorsEnd etc.?
Also, for 'EARLY', you'd probably need to do indoorsStart.substring(6) instead indoorsStart.substring(5) so let's make separate checks for that!
| isInRange( | ||
| monthToIndex.get(indoorsStart)!, | ||
| monthToIndex.get(indoorsEnd)!, | ||
| validIndexes!, | ||
| ) || | ||
| isInRange( | ||
| monthToIndex.get(outdoorsStart)!, | ||
| monthToIndex.get(outdoorsEnd)!, | ||
| validIndexes!, |
There was a problem hiding this comment.
great handle! i was originally thinking that we'd only check
isInRange(
monthToIndex.get(indoorsStart)!,
monthToIndex.get(outdoorsEnd)!,
validIndexes!)
However, this logic seems more accurate. i'll check w POC, but this is great for now
| .filter(filterPlantList) | ||
| .sort((a, b) => a.plant_name.localeCompare(b.plant_name)) | ||
| .map((plant, key) => ( | ||
| //this should display PlantCalendarRows instead of this temporary div | ||
| <div key={key}>{plant.plant_name}</div> | ||
| ))} |
There was a problem hiding this comment.
i'm wondering if we should use a useMemo or do the alphabetical sorting outside of the returned html, i.e. before filtering? @ronniebeggs @pragyakallanagoudar
but this is all right for now!
What's new in this PR 🧑🌾
Description
Screenshots
How to review
Next steps
Relevant links
Online sources
Related PRs
CC: @ccatherinetan