Lf 4742 link ensemble sensor list to map #3702
Conversation
…nt layout is annoying haha
Duncan-Brain
requested review from
SayakaOno and
kathyavini
and removed request for
a team
| style: PropTypes.object, | ||
| farmName: PropTypes.string, | ||
| showVideo: PropTypes.func, | ||
| showClose: PropTypes.func, |
There was a problem hiding this comment.
Should this be onClose or handleClose?
There was a problem hiding this comment.
I kept the existing style of the component 'showVideo', but could update both?
There was a problem hiding this comment.
"showVideo" is a function to show videos (not the icon), but isn't "showClose" used to close the map?
I might have misunderstood something 🤔
There was a problem hiding this comment.
showVideo should have been named "onClickVideo" or "handleVideoClick" in my opinion because yes it is a function to show the videos. I didn't look how hard it would be to rename it.
Once I used it as a boolean to render the icon the name makes sense in that situation as a boolean. Then I kept that convention.
I will just change them both! I have no clue why but the video icon is an <input/> too.
There was a problem hiding this comment.
So they are both named with 'handle' now -- it was easy there was only one other component using it. I also moved the icon away from being an input element and converted the file to TS.
| <DetectedFields t={t} fields={fields} /> | ||
| <TextButton | ||
| className={styles.seeOnMapButton} | ||
| onClick={() => handleSeeOnMap(location)} |
There was a problem hiding this comment.
Location duplicates data already available in GroupedSensors. Could we instead extract the necessary info from GroupedSensors and pass:
{
id,
location_id: id,
point,
type: isSensorArray ? 'sensor_array' : 'sensor',
}
instead of adding Location to useGroupedSensors?
If adding a fake location_id here doesn't feel right, we could add the fake one returned by the backend in useGroupedSensors...
There was a problem hiding this comment.
Good question -- I know I duplicated data and I do not want to do it, but it was purposeful. I struggled with what the nicest way to do it was since we have two systems for using locations now -- selectors and queries. And the old components use the selectors.
I was tempted to just use the selectors and I think I thought if I matched the selectors... since the components were all built to use them .. I could avoid overthinking and "explicitly selecting" the desired data as you suggest. I wanted to just pass it and forget it even it it meant duplicating a little bit of data.
I am worried about eventually dealing with fake location_ids.. but it probably doesnt matter right now. Looking at it again I can probably explicitly choose the parameters as you are suggesting and add all the conditionals to deal with the structure differences... let me try that.
There was a problem hiding this comment.
Okay I used you code here -- the location type needed a name propert, that is the only reason I added it.
| }} | ||
| locations={locations} | ||
| // Choose the active state as the way to view-only locations | ||
| selectedLocationIds={locations.map((l) => l.id)} |
There was a problem hiding this comment.
Should this be l.location_id?
There was a problem hiding this comment.
Thanks! Not sure why it worked before
Description
This creates a LocationViewer component -- it includes a header bar with close button and selected locations.
The full width effect is on the route and not on the component.
Jira link: LF-4742
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: