Current Map Feature Behind Feature Flag #96
Conversation
color booths differently on hover
Was using the Drawer component for the sidebar for both small and large screens, but didnt work that well on desktop and covered parts of map since it has to be positioned absolutely. Switched to just a div that can have its width toggled to 0 with a button. Map width grows to fill remaining space
color booths differently on hover
There was a problem hiding this comment.
Great work guys! Me and August has looked through the code, in general the feature is quite complete I think! The editor for the map seems to work really well and will be of great value for the person inputting the companies. The structure and logic of the map feels good! Nice!
There are quite a lot of smaller things through that needs to be addressed, not all of them necessarily has to be fixed now
Some general feedback:
- Try not to nest components, that will create problems down the road with renders etc
- There seem to be some dead code (out-commented code, unused files or just duplicated logic). Best practice would be to remove any code that is unused.
- Thank you for doing code documentation for parts of the code that might be of a bit higher complexity. It helps everybody understand the code a lot more!
| @@ -1,3 +1,13 @@ | |||
| @tailwind base; | |||
| @tailwind components; | |||
| @tailwind utilities; | |||
|
|
|||
| /* doesnt seem to work */ | |||
There was a problem hiding this comment.
If it doesn't work then we should not add it, are you sure its not doing anything?
There was a problem hiding this comment.
That was for styling elements created by maplibre (the underlying map library used by react-map-gl) that can't be styled through react, like improving the look of the default popup and hiding the "attribution" thing that is always displayed in the bottom right corner of the map. But they don't seem to applied at all even though im pretty sure the selectors are correct, so thought it might be some tailwind-specific issue related to how global styles work (?), but couldn’t figure it out. Kept it because I was planning to come back to it later at some point
| ): FilterItem[] { | ||
| const distinct = new Map<FilterItem["id"], FilterItem>() | ||
| exhibitors.forEach(e => { | ||
| e[key].forEach(item => distinct.set(item.id, item)) |
There was a problem hiding this comment.
Potential source for bug here, if you specify a field which isn't an array, you're going to iterate over undefined values. You have options, either, implement behavior that properly handles this, OR limit the keys that are available, for example only allow employments or industries
There was a problem hiding this comment.
I added an array check to make it safer. The key is defined at the beginning of the file and is limited to only employments or industries.
There was a problem hiding this comment.
Could be wrong but I don't think the runtime check is necessary since passing anything other than "employments" or "industries" will be a type error, and Exhibitor["employments" | "industries"] is known by typescript to be an array, so even adding non-array keys to the definition of FilterKey or changing the definition of Exhibitor would result in a type error in the filters file. So I ended up removing the check again, but let me know if you think this was incorrect
There was a problem hiding this comment.
I agree the checking might be not necessary here, since typescript already has the type check, and we already limited the key. Just to intentionally prevent the array to be null. But in the schema definition we don’t allow null value in Exhibitor object. So I think it is safe enough to not have the checking.
src/app/student/exhibitors/_components/ExhibitorListFilteringHeader.tsx
Outdated
Show resolved
Hide resolved
| zoom: 18 | ||
| } | ||
|
|
||
| export const locations: Location[] = [ |
| const boothsByLocation: Map<LocationId, BoothMap> = new Map( | ||
| locations.map(loc => [loc.id, new Map()]) | ||
| ) | ||
| boothsById.forEach((booth, id) => { | ||
| boothsByLocation.get(booth.location)!.set(id, booth) | ||
| }) |
src/app/student/map/page.tsx
Outdated
|
|
||
| return ( | ||
| // TODO: pt-16 is to account for the navbar, will break if navbar size changes | ||
| <Suspense> |
There was a problem hiding this comment.
Since the entire page is in the suspense does it provide any value to have it? Maybe add a fallback, or extract parts of the page outside the suspense
There was a problem hiding this comment.
Yes the previous Vercel deployment error showed up when we didn't have it, that's why I added it here. It caused by calling useSearchParams() function in MainView.tsx, so basically the whole page is suspense, I could not come up a best extraction for only certain part of the page so far.
| } | ||
| } | ||
| }, | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| plugins: [require("tailwindcss-animate")] | ||
| plugins: [tailwindcssAnimate, containerQueries] |
There was a problem hiding this comment.
Thank you for fixing this 🙏 We want to get rid of commonjs
- removed ExhibitorListFilteringHeader which was not being used anymore - moved ExhibitorFilters from student/components -> student/exhibitors/components since its not used outside exhibitors/ - renamed 'MapListFilteringHeader' -> 'MapFilters' to match 'ExhibitorFilters' - removed map/components/Layers.tsx (code was already moved to map/lib/config.ts)
|
Great progress, I think we're close to being able to merge this, the blocking thing for me is just the nested components now. If all of those are addressed we can fix the rest at a later point |
We need to merge the map feature into main before it goes too far.
Current functionalities: