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

[Setup] Add sample info view and row description #40

Merged
merged 26 commits into from
Aug 10, 2022
Merged

Conversation

clee088
Copy link
Contributor

@clee088 clee088 commented Jul 19, 2022

Description

This PR adds an information view for a sample and a toggle to view the sample's description in the samples list. Within the information view, the user can toggles between the sample's README and source code.

Linked Issue(s)

  • common-samples/issues/3704

How To Test

  1. Tap on the information icon in a sample's row in the list of samples.
  2. Navigate to a sample and tap on the information icon in the top right of the navigation view.
  3. Toggle between the README and code view.

Screenshots

Screenshots

sample-description
sample-readme
sample-code

To Discuss

When device appearance (light/dark) changes while viewing a sample's code, the colors only change within the current viewport. There is a line that is still in the old color scheme. This shouldn't be a huge issue, but as a workaround, I could change it so that when the color scheme changes, it loads a different CSS file. Currently, there is one CSS file and uses @media (prefers-color-scheme: dark). This would reload cause the HTML to reload and the current scroll position of the web view would not be saved.

@clee088 clee088 requested a review from yo1995 July 19, 2022 18:36
WebView(htmlString: readmeHTML ?? errorHTML)
.zIndex(informationMode == .readme ? 1 : 0)
WebView(htmlString: codeHTML ?? errorHTML)
.zIndex(informationMode == .code ? 1 : 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would the opacity modifier be better? Also just to note, I decided to use a ZStack as opposed to an if block because the web view would reload when the picker changes, causing the scroll position to be lost. A minor issue, but personally I would prefer it to be kept.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit shocked to see actually the webview scroll position doesn't change in README view when I switch between different source code files. 🤔 Can you confirm that?

demo.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this makes sense. I also tried it out when adding the picker and experienced the same behavior. The web view for the README is a separate WebView instance and only its opacity is being changed when the information mode changes. When the picker changes the selected code file, the codeHTML changes, updating the web view presenting the code HTML. The README's HTML doesn't change, so its web view doesn't update and its scroll position is saved--it's just "hidden" with zero opacity.

Copy link
Collaborator

@yo1995 yo1995 left a comment

Choose a reason for hiding this comment

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

Haven't finished reviewing yet, some preliminary comments.

ref: Some info can be found in Esri/arcgis-runtime-samples-ios#899

Samples.xcodeproj/project.pbxproj Show resolved Hide resolved
Shared/Supporting Files/Views/SampleList.swift Outdated Show resolved Hide resolved
Shared/Supporting Files/Web/xcode.css Show resolved Hide resolved
Shared/Supporting Files/Web/info.css Show resolved Hide resolved
Shared/Supporting Files/Web/highlight.min.js Outdated Show resolved Hide resolved
yo1995
yo1995 previously approved these changes Aug 5, 2022
Shared/Supporting Files/Views/SampleInfoView.swift Outdated Show resolved Hide resolved
Shared/Supporting Files/Views/WebView.swift Outdated Show resolved Hide resolved
yo1995
yo1995 previously approved these changes Aug 5, 2022
vquach2404
vquach2404 previously approved these changes Aug 5, 2022
yo1995
yo1995 previously approved these changes Aug 8, 2022
vquach2404
vquach2404 previously approved these changes Aug 9, 2022
Shared/Supporting Files/Views/SampleInfoView.swift Outdated Show resolved Hide resolved
Shared/Supporting Files/Views/SampleList.swift Outdated Show resolved Hide resolved
Comment on lines +76 to +81
if isShowingDescription {
Text(sample.description)
.font(.caption)
.foregroundColor(.secondary)
.transition(.move(edge: .top).combined(with: .opacity))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's too bad popovers on iPhones show as sheets because this would be a nice candidate otherwise.

Screen Shot 2022-08-09 at 13 22 30

Shared/Supporting Files/Web/info.css Outdated Show resolved Hide resolved
Shared/Supporting Files/Web/xcode.css Outdated Show resolved Hide resolved
Shared/Supporting Files/Web/xcode.css Outdated Show resolved Hide resolved
@dfeinzimer
Copy link
Contributor

Not directly related to this PR, but iPad shows a blank screen on startup. Should the NavigationView in ContentView.swift have .navigationViewStyle(.stack) on it?

Screen Shot 2022-08-09 at 13 26 50

@clee088 clee088 dismissed stale reviews from vquach2404 and yo1995 via 47e6204 August 9, 2022 20:36
@yo1995
Copy link
Collaborator

yo1995 commented Aug 9, 2022

Not directly related to this PR, but iPad shows a blank screen on startup. Should the NavigationView in ContentView.swift have .navigationViewStyle(.stack) on it?

I think this is Apple's intended way. If the Photos app is opened on iPad vertical with no photos inside, it will show a similar blank screen. Tapping the upper left corner reveals the sidebar.

Simulator Screen Shot - iPad Pro (11-inch) (3rd generation) - 2022-08-09 at 13 36 57

@dfeinzimer
Copy link
Contributor

I think this is Apple's intended way. If the Photos app is opened on iPad vertical with no photos inside, it will show a similar blank screen. Tapping the upper left corner reveals the sidebar.

Interesting point. I can see how it can be interpreted that way, but does that hold true if Photos are added?

In the case of this app, a user won't ever add samples, will they? I think either is fine and it's your call in the end.. just wanted to note it down.

@yo1995
Copy link
Collaborator

yo1995 commented Aug 9, 2022

That's a good point. I'll jot down a card in this repo to show the sidebar by default, and later on poll what others think. 👍

Also, with iOS 16 introduction of NavigationSplitView view, maybe in the future they'll make it easier to show the sidebar in SwiftUI, instead of hacking into UIKit. Ref https://stackoverflow.com/questions/68807418/how-to-show-in-swiftui-the-sidebar-in-ipad-and-portrait-mode

@yo1995 yo1995 changed the title [New] Add sample info view and row description [Setup] Add sample info view and row description Aug 10, 2022
@yo1995 yo1995 requested a review from dfeinzimer August 10, 2022 18:05
@yo1995 yo1995 removed the request for review from mhdostal August 10, 2022 20:34
@clee088 clee088 merged commit 018cedb into v.next Aug 10, 2022
@clee088 clee088 deleted the clee/New-SampleInfo branch August 10, 2022 20:49
@yo1995 yo1995 moved this from In Progress to Done in SwiftUI Sample Viewer prototyping Aug 10, 2022
@yo1995 yo1995 mentioned this pull request Nov 10, 2022
@yo1995
Copy link
Collaborator

yo1995 commented Mar 22, 2023

a warning to investigate

2023-03-21 19:05:42.953131-0600 Samples[16092:6402401] [assertion] Error acquiring assertion: <Error Domain=RBSServiceErrorDomain Code=1 "target is not running or doesn't have entitlement com.apple.runningboard.assertions.webkit" UserInfo={NSLocalizedFailureReason=target is not running or doesn't have entitlement com.apple.runningboard.assertions.webkit}>
2023-03-21 19:05:42.953211-0600 Samples[16092:6402401] [ProcessSuspension] 0x13101c360 - ProcessAssertion::acquireSync Failed to acquire RBS assertion 'ConnectionTerminationWatchdog' for process with PID=16139, error: Error Domain=RBSServiceErrorDomain Code=1 "target is not running or doesn't have entitlement com.apple.runningboard.assertions.webkit" UserInfo={NSLocalizedFailureReason=target is not running or doesn't have entitlement com.apple.runningboard.assertions.webkit}
2023-03-21 19:05:42.954397-0600 Samples[16092:6402401] [assertion] Error acquiring assertion: <Error Domain=RBSAssertionErrorDomain Code=2 "Specified target process does not exist" UserInfo={NSLocalizedFailureReason=Specified target process does not exist}>
2023-03-21 19:05:42.954445-0600 Samples[16092:6402401] [ProcessSuspension] 0x13101c420 - ProcessAssertion::acquireSync Failed to acquire RBS assertion 'WebProcess Suspended Assertion' for process with PID=16139, error: Error Domain=RBSAssertionErrorDomain Code=2 "Specified target process does not exist" UserInfo={NSLocalizedFailureReason=Specified target process does not exist}
2023-03-21 19:05:42.974866-0600 Samples[16092:6402401] [assertion] Error acquiring assertion: <Error Domain=RBSServiceErrorDomain Code=1 "target is not running or doesn't have entitlement com.apple.runningboard.assertions.webkit" UserInfo={NSLocalizedFailureReason=target is not running or doesn't have entitlement com.apple.runningboard.assertions.webkit}>
2023-03-21 19:05:42.974937-0600 Samples[16092:6402401] [ProcessSuspension] 0x13101c4e0 - ProcessAssertion::acquireSync Failed to acquire RBS assertion 'WebProcess Suspended Assertion' for process with PID=16140, error: Error Domain=RBSServiceErrorDomain Code=1 "target is not running or doesn't have entitlement com.apple.runningboard.assertions.webkit" UserInfo={NSLocalizedFailureReason=target is not running or doesn't have entitlement com.apple.runningboard.assertions.webkit}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants