Skip to content

Add visionOS support to UtilityNetworkTrace #1011

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

Merged
merged 9 commits into from
Jan 22, 2025

Conversation

philium
Copy link
Contributor

@philium philium commented Jan 16, 2025

No description provided.

@philium philium self-assigned this Jan 16, 2025
dfeinzimer
dfeinzimer previously approved these changes Jan 16, 2025
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.

Some feedback:

  • This can be removed for visionOS so the button has a default circle around it.

  • In the example file, the UtilityNetworkTrace could use a bit of top padding since it is cramped on the top of the floating panel

  • The "Cancel Starting Point Selection" is a bit of an eye sore and should look like this. Where the background of the button is red and the text is white.
    Screenshot 2025-01-17 at 9 53 53 AM

  • Under "Advanced Options", it isn't clear the "Name" row is selectable to type in text. So adding a hover effect to the TextField there could probably help.

@zkline101
Copy link
Contributor

Also, I was expecting each section to look like it does in iOS:
Simulator Screenshot - iPad Pro 13-inch (M4) - 2025-01-17 at 10 12 39

where there is a background color for each row. Similar to how the settings app is in visionOS or iOS. I'm assuming there is something in the code that is preventing that default behavior on visionOS .

@philium
Copy link
Contributor Author

philium commented Jan 17, 2025

The "Cancel Starting Point Selection" is a bit of an eye sore and should look like this. Where the background of the button is red and the text is white.

The dialogue is created using the built-in confirmation dialog and the delete button has a destructive role. While it may look odd compared to other buttons, it matches other system confirmation dialogues, such as this one:

image

@philium philium force-pushed the philium/UtilityNetworkTrace-visionOS-support branch from edd8f70 to 4e4c568 Compare January 17, 2025 22:58
@philium
Copy link
Contributor Author

philium commented Jan 17, 2025

I suspect that the lists aren't styled as grouped because the list is not the root of the view in the navigation stack.

@philium philium requested a review from zkline101 January 17, 2025 23:00
@zkline101
Copy link
Contributor

I suspect that the lists aren't styled as grouped because the list is not the root of the view in the navigation stack.

Should we try that? Or do you think that would require too many changes?

@zkline101
Copy link
Contributor

Please remove#if !os(visionOS) fromUtilityNetworkTraceViewModelTests so those tests can be run on visionOS.

@zkline101
Copy link
Contributor

@mhdostal @dfeinzimer

I am helping Phil finish these PRs while he is out on vacation, can you review this? Thank you.

@zkline101
Copy link
Contributor

I suspect that the lists aren't styled as grouped because the list is not the root of the view in the navigation stack.

I created an issue to come back to this.

dfeinzimer
dfeinzimer previously approved these changes Jan 21, 2025
mhdostal
mhdostal previously approved these changes Jan 21, 2025
@zkline101 zkline101 dismissed stale reviews from mhdostal and dfeinzimer via 9280aae January 21, 2025 20:16
@zkline101
Copy link
Contributor

Fixed merge conflict, please re-review.

@zkline101 zkline101 merged commit bd314de into v.next Jan 22, 2025
@zkline101 zkline101 deleted the philium/UtilityNetworkTrace-visionOS-support branch January 22, 2025 01:30
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.

4 participants