Skip to content

Ej/background maps settings flag#737

Closed
lightlii wants to merge 35 commits intomasterfrom
ej/background-maps-settings-flag
Closed

Ej/background maps settings flag#737
lightlii wants to merge 35 commits intomasterfrom
ej/background-maps-settings-flag

Conversation

@lightlii
Copy link
Copy Markdown
Contributor

@lightlii lightlii commented Jul 12, 2023

Adds 'Experiments' list to settings menu with Background Maps as (currently only) option.
When Background Maps flag is activated, Background maps becomes an available option in settings.

closes #733

Preview

Screenshot from 2023-07-17 12-33-16
Screenshot from 2023-07-17 12-33-10
Screenshot from 2023-07-17 12-33-25

@lightlii lightlii requested review from ErikSin and gmaclennan and removed request for gmaclennan July 12, 2023 13:57
@lightlii lightlii marked this pull request as ready for review July 17, 2023 10:36
Copy link
Copy Markdown
Contributor

@ErikSin ErikSin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! I left some minor comments/suggestions

2 things:

  1. Once you rebase/merge from master, I would like to give it one more quick look before you merge.
  2. I added alot of maps in testing. When it is loading the list of maps, it blocks the UI. I think that is fine when it is initially loading, but Tan stack query makes the API call everytime the window is out of focus and refocused, causing the UI to be blocked. I think we should disable invalidating the query. And when the users uploads a new map, we can use the refetch function to refresh the list. That way the list is not constantly trying to refetch even when we know there is no new data.

Comment thread src/renderer/components/BackgroundMaps/BackgroundMapInfo.js
Comment thread src/renderer/components/BackgroundMaps/SidePanel.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the SettingsList is only being used by the Experiments Menu. Is that the case, I think we need to rename it as "ExperimentsList" or something more general like "listItem". Right now it implies it is being used for the settings menu not the experiments menu.

Comment thread src/renderer/components/dialogs/ImportMapStyle.js Outdated
Comment thread src/renderer/components/dialogs/ImportMapStyle.js Outdated
Comment thread src/renderer/components/SettingsView/index.js Outdated
@lightlii
Copy link
Copy Markdown
Contributor Author

Hey I think I've added what you requested in (2) in your comment above. So it's ready to look at again I'd say!

@lightlii
Copy link
Copy Markdown
Contributor Author

Oops sorry, nope I missed error handling in ImportMapStyle, I can finish that up first thing tomorrow

@lightlii lightlii force-pushed the ej/background-maps-settings-flag branch from dec835d to 9fd494e Compare July 27, 2023 12:42
@lightlii
Copy link
Copy Markdown
Contributor Author

@ErikSin I've rebased from master, so this is ready for a last look over

@ErikSin
Copy link
Copy Markdown
Contributor

ErikSin commented Aug 2, 2023

Im unable to open it. Are you able to open it on your local device?

@lightlii
Copy link
Copy Markdown
Contributor Author

lightlii commented Aug 7, 2023

Im unable to open it. Are you able to open it on your local device?

I don't have any issues running it. Lets look at that together when we call later

@ErikSin
Copy link
Copy Markdown
Contributor

ErikSin commented Aug 9, 2023

There are some import errors that the linter and vs code are catching

@lightlii lightlii requested a review from ErikSin August 28, 2023 15:21
@lightlii
Copy link
Copy Markdown
Contributor Author

These changes are in #744 so @ErikSin & I agreed to close and only review #744

@lightlii lightlii closed this Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update list map styles PR

2 participants