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

Update Documentation for OnboardingView screen #40

Closed

Conversation

akanshyabhat
Copy link
Contributor

@akanshyabhat akanshyabhat commented Feb 29, 2024

Update Documentation for OnboardingView screen

♻️ Current situation & Problem

I noticed that the documentation for the initializers and overall descriptions of this screen were a bit unclear vague when I was trying to use them.

📚 Documentation

  • Added additional details for each initializer and what differentiates each one
  • Added more of a specific description to the overall view

📝 Code of Conduct & Contributing Guidelines

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

@Supereg
Copy link
Member

Supereg commented Mar 1, 2024

@PSchmiedmayer the set of required checks is a bit weird here. The checks getting run are slightly different to the ones that the branch protection rule enforces.

Copy link

codecov bot commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.75%. Comparing base (4971a82) to head (a82db3d).

❗ Current head a82db3d differs from pull request most recent head bb1d154. Consider uploading reports for the commit bb1d154 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #40   +/-   ##
=======================================
  Coverage   76.75%   76.75%           
=======================================
  Files          22       22           
  Lines        1148     1148           
=======================================
  Hits          881      881           
  Misses        267      267           
Files Coverage Δ
Sources/SpeziOnboarding/OnboardingView.swift 58.58% <ø> (ø)

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 4971a82...bb1d154. Read the comment docs.

@PSchmiedmayer
Copy link
Member

@Supereg We already adjusted them for #39 and once this is merged we could merge this PR.

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.

@akanshyabhat Thank you for the addition. I added some first review comments; looking forward to the next version of the PR.

Sources/SpeziOnboarding/OnboardingView.swift Outdated Show resolved Hide resolved
Sources/SpeziOnboarding/OnboardingView.swift Show resolved Hide resolved
Copy link
Member

@philippzagar philippzagar left a comment

Choose a reason for hiding this comment

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

Thanks @akanshyabhat for the PR! Just had some minor comments!

Sources/SpeziOnboarding/OnboardingView.swift Outdated Show resolved Hide resolved
Sources/SpeziOnboarding/OnboardingView.swift Outdated Show resolved Hide resolved
Sources/SpeziOnboarding/OnboardingView.swift Outdated Show resolved Hide resolved
Sources/SpeziOnboarding/OnboardingView.swift Outdated Show resolved Hide resolved
@akanshyabhat
Copy link
Contributor Author

Thanks @akanshyabhat for the PR! Just had some minor comments!

Thank you so much for your feedback @philippzagar! I just implemented it all and would appreciate it if you could take another look.

Copy link
Member

@philippzagar philippzagar left a comment

Choose a reason for hiding this comment

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

Thanks for incorporating the change requests, feel free to merge the PR @akanshyabhat 🚀

@akanshyabhat
Copy link
Contributor Author

Thanks for incorporating the change requests, feel free to merge the PR @akanshyabhat 🚀

Screenshot 2024-03-04 at 9 33 31 PM

Thank you so much @philippzagar ! On my end, it says that merging is blocked and that there are two workflows awaiting approval. I'm also not quite sure how to show that I made the changes that Paul had requested.

@philippzagar
Copy link
Member

@akanshyabhat Just approved the workflows, they should complete within a couple of minutes

@akanshyabhat
Copy link
Contributor Author

@philippzagar I'm still getting a message saying that I can't merge until the requested changes (left by Paul) have been addressed. I'm not quite sure how I can address that beyond resolving the conversations which I've already done.

@philippzagar philippzagar added the enhancement New feature or request label Mar 5, 2024
@philippzagar philippzagar dismissed PSchmiedmayer’s stale review March 5, 2024 05:44

I reviewed the PR, feedback of Paul has been addressed.

@philippzagar philippzagar removed the request for review from PSchmiedmayer March 5, 2024 05:44
@philippzagar
Copy link
Member

@philippzagar I'm still getting a message saying that I can't merge until the requested changes (left by Paul) have been addressed. I'm not quite sure how I can address that beyond resolving the conversations which I've already done.

The issue is resolved now, I dismissed Paul's review

@akanshyabhat akanshyabhat reopened this Mar 5, 2024
@akanshyabhat
Copy link
Contributor Author

Thank you so much!

@philippzagar
Copy link
Member

@akanshyabhat Your commits on the feature branch are unsigned, please ensure that you properly set up your local commit signing: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

@akanshyabhat
Copy link
Contributor Author

@akanshyabhat Your commits on the feature branch are unsigned, please ensure that you properly set up your local commit signing: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

@philippzagar I talked with Andreas today and he told me that we don’t need to have commits signed. I can make the necessary change but I’m not sure how to revise an old commit with a signature. Do you have any guidance regarding that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants