Sort MapSwitcher options #1088
Sort MapSwitcher options #1088
Conversation
| @@ -29,6 +29,11 @@ class MapSwitcher extends React.PureComponent { | |||
|
|
|||
| handleLoading(maps) { | |||
| let { activeMap } = this.appModel.config; | |||
|
|
|||
| maps.sort((a, b) => | |||
There was a problem hiding this comment.
I'm sorting the array in place instead of copying, some people don't like that but in this case I think it's reasonable instead of copying what seem to be rather complex objects.
There was a problem hiding this comment.
I'll approve this one, but would lika a review from @jesade-vbg or @jacobwod as well (since i haven't used the map-switcher-functionality lately).
The reasoning behind the need for another review is that i am unsure if mapConfigurationTitle could ever be undefined? (If it could we would go out with a bang when running the localeCompare).
There's no fallback in the backend when setting it as far as i can se.
|
Another thought: maybe the sorting of the maps should be done in the backend instead of in the client in the future... Seems like unnecessary load for the client. |
|
Thanks for the feedback @Hallbergs, here's my reasoning: 1. Potentially Good point. I took a quick look if there were any existing falsy checks for the title in the component, it didn't look like that so I went with the naive approach. 2. Sorting options in the backend I tend to favour doing as much compute on the backend as possible, but client compute is also free compute, so I guess there's always a trade-off. With this being such a heavy application I think doing as much as possible on the backend seems reasonable. |
The fact that you reasoned about it is good enough for me! Good job :) Always fun to see a new contributer, welcome! |
Note: I did some minor refactoring that was out of scope, it's in a different commit in case it causes confusion