Ej/background map selection #744
Conversation
…hat's being saved (JSON or string)
| <BackgroundMapInfo | ||
| key={offlineMap.id} | ||
| size={offlineMap.bytesStored} | ||
| id={offlineMap.id} | ||
| unsetMapValue={unsetMapValue} | ||
| url={offlineMap.url} |
There was a problem hiding this comment.
Default map should be the available on this list.
Also if this list is longer than the screen, the user is unable to scroll
src/renderer/components/MapFilter/MapView/BackgroundMapSelector.js
Outdated
Show resolved
Hide resolved
| <Box className={classes.styleRow}> | ||
| {mapStyles | ||
| .filter(({ isImporting }) => !isImporting) | ||
| .map(({ id, url, name }) => { | ||
| const isSelected = mapStyle === id | ||
| return ( | ||
| <React.Fragment key={id}> | ||
| <div className={classes.mapCardWrapper}> | ||
| <MapPreviewCard | ||
| onClick={() => { | ||
| if (isSelected) return | ||
| dismiss() | ||
| setMapStyle(id) | ||
| }} | ||
| selected={isSelected} | ||
| styleUrl={url} | ||
| title={name} | ||
| /> | ||
| </div> | ||
| </React.Fragment> | ||
| ) | ||
| })} | ||
| </Box> |
There was a problem hiding this comment.
if there are lots of map the user is unable to scroll to see them. When you convert this to a dialog, you may need to add a scroll view, but the user should be able to see the majority of the maps if you make it a full screen dialog
There was a problem hiding this comment.
I think the architecture of this is a little confusing.
We could simplify this by just exporting 1 hook, a react query (server state) hook.
Thisreact-query-hook that always returns the list of available styles. This would mean that when background maps is disabled, it just returns an array with 1 map style. And when background maps is enabled, it returns an array (which also includes the default map style). This hook should encapsulate all this logic. If returning the react-query is too complicated, you could just return the list of styles and encapsulate the loading state inside of the hook (aka the hook does not have to return a react-query). It should also handle whether experiments flag is turned on or not.
And then the zustand persisted store, useBackgroundMapStore, should handle which map style is used. What that does mean is the map should not be shown while the server state hook is loading. Therefore zustand does not have to deal with any of the loading state, and only deals with returning the persisted map style as chosen by the user. As well the zustand store does not need to deal with experimentsFlag, and is scoped to just serving the map.
Right now, the hook serving the mapStyle and the hook dealing with server state are both dealing loading state AND the experiments flag, which will make it hard to maintain in the future . As well, because there are so many hooks, the data that is being returned is a combination of the hooks, that again will be hard to maintain in the future.
Essentially there should be 2 exported hooks:
- zustand state hook that returns the map being used (which is already created)
- A hook that serves list of avialable maps, that encapsulates
experimentsFlagandloading state
And those can be used in both your modal and in the setting/background map page, without having to transform any data.
|
I've made a bit of a refactor but I'm not sure it's exactly what you meant. We still need the |
* persisted state * chore: translations * chore: create default map export & adapt code --------- Co-authored-by: lightlii <[email protected]>
| @@ -18,29 +18,49 @@ const m = defineMessages({ | |||
| offlineBackgroundMapName: 'Offline Map' | |||
| }) | |||
|
|
|||
| export const useMapStylesQuery = () => { | |||
| export const useMapStylesQuery = (enabled = true) => { | |||
There was a problem hiding this comment.
I dont think I understand what the difference between enable and backgroundMapsEnabled
There was a problem hiding this comment.
enable enables the query. It allows us to disable the querying and manually call refetch when we want the data. I can't find the original ticket but it was one of the first thing you asked me to add on this feature!
| <div> | ||
| {!isDefault && ( | ||
| <Button variant='outlined' onClick={() => deleteMap()}> | ||
| <DeleteIcon className={classes.icon} /> | ||
| <Typography style={{ textTransform: 'none' }} variant='subtitle2'> | ||
| {t(m.deleteStyle)} | ||
| </Typography> | ||
| </Button> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
this is currently not working, but I believe this is a map server thing, so just remove it for now (and the user will not be able to delete a map)
There was a problem hiding this comment.
this is working fine for me, lets check together later whats going on
There was a problem hiding this comment.
(I added a little check so it will set the the default map if you delete the current selected one)
Currently WIP. PR #741 contains work that needs confirming and merging so this can be completed