chore: list styles, BG Map Management

[ErikSin]

List Styles

Screen built for managing all background maps/styles

Screens and Api calls for listing background maps

Buttons for deleting map (not yet functional)

Figma:

https://www.figma.com/file/sxz2NvysPVab6ZTqs6f57K/Cobra-Collective?node-id=245%3A1327

[ErikSin]

Does this look correct? Currently it is not showing any style, just a green box

[achou11]

this seems correct! curious about what dataset you're using for your example.

perhaps unrelated, but will note that the way you're instantiating mapbox-react-gl is a little odd. Usually you'd create the instance outside of react i.e. not in a component like you're doing in line 30. See the example from the docs: https://github.com/alex3165/react-mapbox-gl#getting-started

[gmaclennan]

Yes creating an instance of Mapbox-gl is expensive (which is what creating an instance of mapbox-react-gl will do) so you should use a single instance for all maps rather than a different instance for each map.

[ErikSin]

So I created one instance for my two components here. Should I extend that further and use that same instance for the mapview.js (which is actually instantiated in a React compnonent and memoized as well, but fixing that is out of scope for this PR)?

[achou11]

i wouldn't touch that one because

1. out of scope for this PR

2. Wondering if there's some context that i'm missing for that case to instantiate inline. Looks like it relies on a couple of props (basically the access token and the type of screen it's rendered in e.g. map printer), so i'm thinking that that was a pretty intentional (but not ideal) choice. seems like that could be refactored to be improved, but not worth doing in this PR since it's out of scope

but good catch! maybe an issue would be worth writing up for it

[achou11]

Are the fade animations part of the design spec? if not, would say to omit them from this PR because they feel unnecessary and we don't use them elsewhere in the app at the moment

[achou11]

would make this variable a little more descriptive or just inline it to the useState initializer.

[ErikSin]

This is actually a very annoying thing about jsdoc. The typescript equivalent would be useState<SettingsTabs['tabId']|false>(false) In order to do this with JSdoc, i need to initialize the first state of the useState() above it as I did here.

[ErikSin]

I renamed it so it is a little clearer: 73472a2

[achou11]

this state resetting code isn't necessary because you remount this component whenever it's navigated to i.e. in Home.js you do tabIndex === 4 && <Settings ... />

you don't need the const [settingsReset, setSettingsReset] = React.useState(false) in Home.js as a result and can remove the associated props from this component in that case.

tangentially, wonder if it makes sense to not conditionally render the Settings tab (like we do for the other app tabs) because that way you preserve the state when returning to it, which may be desirable from a UX perspective (as opposed to always starting in the reset state when navigating to it). This behavior could be useful when importing a map and you want to navigate elsewhere in the app without losing anything about the map import you started.

[ErikSin]

So the reason for setReset() was if the user wanted to click "settings" again to reset. When background maps tab is open, it takes over the settings page completely. The reset variable made it possible for the user to click the settings tab again to exit the background maps tab. That being said, if we don't want to conditionally render it, then we can't have both, since clicking "settings" will bring you back to where you were in the settings instead of resetting the state

[ErikSin]

took out setReset and made the rendering NOT conditional: 73472a2

[achou11]

ah i see. makes more sense to me now

if the user wanted to click "settings" again to reset.

is this necessary given that the background maps settings screen has a back button in the sidebar? just thinking that adding the additional logic to cover this use case may not be worth it for now if a mechanism already exists

[ErikSin]

it was slightly confusing to hit settings and see the background maps tab. But if we don't want to conditionally render it, then we have to get rid of setReset() (Or add even more complicated logic) anyways

[achou11]

just remembered that you can reset the react state by providing a new key prop value, so it may be possible to avoid extra reset code and just providing a new value for the Settings tab panel key prop in Home, if it's useful

EDIT: Probably not a useful tip since we're using a TabPanel and providing the component as a prop there nevermind, i thought TabPanel was coming from Material, but it's our own thing, so tip may apply

EDIT 2: I briefly tried this out but didn't get it to work as expected, so probably not worth trying

[achou11]

think it makes sense to avoid any reset behavior for now since it's less code to work with. if the reset stuff is desired, should be easy to add later on

[ErikSin]

just to clarify, we can't really have both the "saving of the tab state", and "resetting settings on click". Either we click settings and it goes to the saved tab state Or we click settings and it resets the tab state

[achou11]

yeah that's how I understood it and I'm leaning towards the former (preserve state) unless you're okay with adding the code to account for the latter

[achou11]

Icon and label should be vertically centered with each other:

[ErikSin]

c607d02

[ErikSin]

for what its worth, this was annoyingly hard to do with MUI ..... 😂