Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mhd/overview map refactor #41

Merged
merged 6 commits into from
Apr 8, 2022
Merged

Mhd/overview map refactor #41

merged 6 commits into from
Apr 8, 2022

Conversation

mhdostal
Copy link
Member

@mhdostal mhdostal commented Apr 6, 2022

This refactors the OverviewMap. Changes include:

  • @State, @binding, and other property wrappers should be on the same line as the variable declaration.
  • Change the OverviewMap view modifiers to use trailing closures(?), which is more readable and also appears to have fixed the indentation issues.
  • Change the massive width segmented control to a nav bar trailing button with context menu to choose between map and scene.
    image

#37

cc: @rolson

@mhdostal mhdostal self-assigned this Apr 6, 2022
@mhdostal mhdostal changed the base branch from main to v.next April 6, 2022 17:01
@mhdostal mhdostal mentioned this pull request Apr 6, 2022
5 tasks
dfeinzimer
dfeinzimer previously approved these changes Apr 7, 2022
Copy link
Collaborator

@dfeinzimer dfeinzimer left a comment

Choose a reason for hiding this comment

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

Looks good! Just the one suggestion

Sources/ArcGISToolkit/Components/OverviewMap.swift Outdated Show resolved Hide resolved
Co-authored-by: David Feinzimer <dfeinzimer@gmail.com>
Co-authored-by: Zachary A Kline <zkline@esri.com>
Copy link
Contributor

@zkline101 zkline101 left a comment

Choose a reason for hiding this comment

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

🤙

@mhdostal mhdostal merged commit e72ff5a into v.next Apr 8, 2022
@mhdostal mhdostal deleted the mhd/OverviewMap_Refactor branch April 8, 2022 16:03
@mhdostal
Copy link
Member Author

mhdostal commented Apr 8, 2022

@dfeinzimer @zkline101 Thank you!

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.

3 participants