Conversation
…re/us-4.5-indoor-points-of-interest-for-services
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
khalillabban
left a comment
There was a problem hiding this comment.
Incredible work, Nick. Just a few changes that you can definitely debate if you think they're unnecessary.
| [buildingName], | ||
| ); | ||
|
|
||
| const allPOIs = useMemo( |
There was a problem hiding this comment.
Can we confirm whether getIndoorPOIs(buildingName) is expensive enough to justify useMemo here? If this is just a static lookup, a plain const may be simpler and easier to read. Not blocking, just checking whether memoization is actually buying us anything. This is a possibility:
const allPOIs = buildingName ? getIndoorPOIs(buildingName) : [];
There was a problem hiding this comment.
Agreed, getIndoorPOIs is just a static lookup so there's no need for memoization. Replaced it with a plain const.
|
@khalillabban all review comments have been addressed in the latest commits:
|
|
khalillabban
left a comment
There was a problem hiding this comment.
Incredible work gentlemen!



Added a toggleable points of interest (POI) overlay to the indoor map, fixed indoor navigation connectivity for VL and MB buildings, and improved the MB floor map rendering.