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

Updated Spezi documentation #68

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Updated Spezi documentation #68

wants to merge 10 commits into from

Conversation

AdritRao
Copy link
Member

@AdritRao AdritRao commented Jan 22, 2024

Updated Spezi documentation

♻️ Current situation & Problem

Currently, the individual module pages are auto-generated in the Spezi documentation. These pages do not contain iPhone screenshots nor a brief description of the module's purpose.

⚙️ Release Notes

  • Added links to the specific modules below the screenshots in the overview: TemplateApplication.md
  • Added the screenshots along with a brief description to the individual module pages (e.g. Consent.swift)

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you @AdritRao for starting to work on this. Goes in the right direction and will be very helpful for people to get started 🚀

@PSchmiedmayer
Copy link
Member

@AdritRao Wanted to follow-up on this PR; should we continue to push for this here or would it make sense to close the PR?

@AdritRao
Copy link
Member Author

AdritRao commented Feb 4, 2025

Hi @PSchmiedmayer, I have addressed the comments in the latest commit. Thank you!

@PSchmiedmayer PSchmiedmayer marked this pull request as ready for review February 4, 2025 19:25
@PSchmiedmayer PSchmiedmayer added enhancement New feature or request documentation Improvements or additions to documentation and removed enhancement New feature or request labels Feb 4, 2025
Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @AdritRao!

I had some smaller comments; we might need to double-check that everything is linked correctly in the documentation if we build it on Xcode + work on some of the comments.

Copy link

codecov bot commented Feb 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.73%. Comparing base (0b78038) to head (a6b6081).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #68   +/-   ##
=======================================
  Coverage   86.73%   86.73%           
=======================================
  Files          21       21           
  Lines         693      693           
=======================================
  Hits          601      601           
  Misses         92       92           
Files with missing lines Coverage Δ
TemplateApplication/Contacts/Contacts.swift 91.84% <ø> (ø)
TemplateApplication/Onboarding/Consent.swift 94.12% <ø> (ø)
...eApplication/Onboarding/HealthKitPermissions.swift 95.56% <ø> (ø)
...ateApplication/Onboarding/InterestingModules.swift 100.00% <ø> (ø)
TemplateApplication/Onboarding/Welcome.swift 100.00% <ø> (ø)
TemplateApplication/Schedule/ScheduleView.swift 100.00% <ø> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b78038...a6b6081. Read the comment docs.

AdritRao and others added 4 commits February 4, 2025 13:04
@AdritRao
Copy link
Member Author

AdritRao commented Feb 4, 2025

Thank you for the feedback @PSchmiedmayer! I have addressed them and tested the documentation by building it on Xcode.

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for all the improvements; I had a few small feedback elements that I noticed when rendering the documentation in Xcode; happy to see this merged once the last round of feedback is incorporated.

Thank you @AdritRao for working on this! 🚀

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for all the improvements here, looks nice!

It would be amazing if every view linked here has a proper documentation description similar to the ones in the other views. We should ensure that clicking on a link would show some more hints and suggestions.

///
/// Customize the HealthKitPermissions view to specify your app's usage of HealthKit data to the user.
///
/// You can learn more about [Spezi Onboarding in the GitHub repository](https://github.com/StanfordSpezi/SpeziOnboarding)
Copy link
Member

Choose a reason for hiding this comment

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

Might be a copy and past error here; we should double check this for all files as well 👍

Suggested change
/// You can learn more about [Spezi Onboarding in the GitHub repository](https://github.com/StanfordSpezi/SpeziOnboarding)
/// You can learn more about [Spezi HealthKit in the GitHub repository](https://github.com/StanfordSpezi/Spezi HealthKit)

Comment on lines +20 to +21
/// ![A screenshot of the HealthKitPermissions screen](HealthKitAccess)
/// ![A screenshot of the HealthKitSheet screen](HealthKitSheet)
Copy link
Member

Choose a reason for hiding this comment

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

The screenshots are currently shown in a really large size; it might not be the desirable size. I would suggest that we use a trick of having a @Row with a size of e.g., 3 columns that only contains one or two and therefore scales the images accordingly.

Suggested change
/// ![A screenshot of the HealthKitPermissions screen](HealthKitAccess)
/// ![A screenshot of the HealthKitSheet screen](HealthKitSheet)
/// @Row(numberOfColumns: 3) {
/// @Column(size: 1) {
/// @Image(source: "HealthKitAccess", alt: "HealthKit Onboarding Flow")
/// }
/// @Column(size: 1) {
/// @Image(source: "HealthKitSheet", alt: "Permissions screen of the HealthKit framework")
/// }
/// }

Applies to other parts of the documentation as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants