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

[New] Display dimensions #279

Merged
merged 16 commits into from
Sep 29, 2023
Merged

[New] Display dimensions #279

merged 16 commits into from
Sep 29, 2023

Conversation

CalebRas
Copy link
Contributor

Description

This PR implements Display dimensions in Layers category.
URL to README: README

Linked Issue(s)

  • swift/issues/4586

How To Test

  • Toggle "Dimension Layer" to change the layer's visibility.
  • Toggle "Definition Expression: Dimensions >= 450m" to change the layer's definition expression.

Screenshots

display-dimensions

@CalebRas CalebRas self-assigned this Sep 28, 2023
@CalebRas CalebRas changed the base branch from main to v.next September 28, 2023 17:25
@CalebRas CalebRas requested review from a team, rolson and njarecha and removed request for a team September 28, 2023 17:27
@CalebRas
Copy link
Contributor Author

I am not super happy with the design/implementation of the settings, but I couldn't find anything better since it you can't use toggles within a Menu and add having a half sheet for just two toggles does really make sense to me. So, I would appreciate any feedback you guys have on that!

Copy link
Contributor

@njarecha njarecha 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

@njarecha
Copy link
Contributor

njarecha commented Sep 28, 2023

@CalebRas

I am not super happy with the design/implementation of the settings, but I couldn't find anything better since it you can't use toggles within a Menu and add having a half sheet for just two toggles does really make sense to me. So, I would appreciate any feedback you guys have on that!

The settings is same as old sample . The design does not have the settings button. I'm ok with directly putting the two toggles in the overlay at bottom.

@CalebRas
Copy link
Contributor Author

I went with the overlay design that Nimesh suggested since the implementation is much simpler, has better backwards compatibility, and doesn't distract from the topic of the sample like the "Settings" button implementation did.

Copy link
Contributor

@rolson rolson 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. couple minor suggestions

rolson
rolson previously approved these changes Sep 29, 2023
njarecha
njarecha previously approved these changes Sep 29, 2023
Copy link
Contributor

@njarecha njarecha 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 one suggestion.

Co-authored-by: Nimesh Jarecha <njarecha@esri.com>
@CalebRas CalebRas dismissed stale reviews from njarecha and rolson via 8e1db00 September 29, 2023 21:55
@CalebRas CalebRas merged commit e4ac614 into v.next Sep 29, 2023
1 check passed
@CalebRas CalebRas deleted the Caleb/New-DisplayDimensions branch September 29, 2023 22:10
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.

None yet

3 participants