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

Content adjustment for upcoming onboarding steps #46

Conversation

felixschlegel
Copy link
Contributor

@felixschlegel felixschlegel commented Mar 25, 2024

Content adjustment for upcoming onboarding steps

♻️ Current situation & Problem

Closes #44.
OnboardingStack only allowed modifications to its views (i.e. order and content of the OnboardingStack) when the onboarding process was at its first step (i.e. no navigation operations have occurred). We want to loosen this restriction by allowing modifications to all views that are ahead of the current onboarding step.

⚙️ Release Notes

  • OnboardingNavigationPath.updateViews: update all views that are ahead of current onboarding step instead of only updating views on the first onboarding step

📚 Documentation

  • No changes to public interface

✅ Testing

  • Create UI Test

📝 Code of Conduct & Contributing Guidelines

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

Motivation:

Closes StanfordSpezi#44.
`OnboardingStack` only allowed modifications to its views (i.e. order
and content of the `OnboardingStack`) when the onboarding process was
at its first step (i.e. no navigation operations have occured).
We want to loosen this restriction by allowing modifications to all
views that are ahead of the current onboarding step.

Modifications:

* `OnboardingNavigationPath.updateViews`: update all views that are
  ahead of current onboarding step instead of only updating views on the
  first onboarding step
@philippzagar
Copy link
Member

@felixschlegel Thanks for the PR; it looks quite straightforward already! 🚀

One random thought: We probably should get rid of "duplicate" onboardingStepsOrder array and onboardingSteps dictionary. The reason why I introduced it back then was for performance reasons (no need to iterate through the onboardingStepsOrder array when accessing a specific step), however, in retrospect, I'm not sure if that actually was the best way to solve this. Realistically speaking, the OnboardingStack contains just a handful of Views, therefore making the overhead of iterating through the onboarding array negligible. Maybe something like an OrderedDictionary would work out?
Let me know what you think.

@philippzagar philippzagar added the enhancement New feature or request label Apr 3, 2024
Modifications:

* `TestApp`: add new `OnboardingCustomToggleTestView` to change state of
  `showConditionalView` when already progressed in the `OnboardingStack`
* add new test case `testDynamicOnboardingFlow3()` that clicks through
  the onboarding process and changes `showConditionalView` in
  `OnboardingCustomToggleTestView` before onboarding is completed
Modifications:

* `Package.swift`: add dependency to `apple/swift-collections`
* `OnboardingNavigationPath`: merge `onboardingSteps` and `onboardingStepsOrder` into one using a
  `swift-collections` `OrderedDictionary`
@felixschlegel felixschlegel removed their assignment Apr 11, 2024
@felixschlegel felixschlegel marked this pull request as ready for review April 11, 2024 17:08
@felixschlegel
Copy link
Contributor Author

@felixschlegel Thanks for the PR; it looks quite straightforward already! 🚀

One random thought: We probably should get rid of "duplicate" onboardingStepsOrder array and onboardingSteps dictionary. The reason why I introduced it back then was for performance reasons (no need to iterate through the onboardingStepsOrder array when accessing a specific step), however, in retrospect, I'm not sure if that actually was the best way to solve this. Realistically speaking, the OnboardingStack contains just a handful of Views, therefore making the overhead of iterating through the onboarding array negligible. Maybe something like an OrderedDictionary would work out? Let me know what you think.

Great point! Have adjusted the code accordingly.

Furthermore, I added a new UI Test assuring that steps ahead of the current onboarding steps can be modified.

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.

@felixschlegel Thank you for the improvements; the changes look great to me. Thanks for adding the UI tests. Let me know once you think that we can merge the PR.

We have some unfortunate issues with codecov and are evaluating how we will continue with the tool beyond the issues here. I can merge the PR with admin permissions once you think it is complete 👍

@felixschlegel
Copy link
Contributor Author

@felixschlegel Thank you for the improvements; the changes look great to me. Thanks for adding the UI tests. Let me know once you think that we can merge the PR.

We have some unfortunate issues with codecov and are evaluating how we will continue with the tool beyond the issues here. I can merge the PR with admin permissions once you think it is complete 👍

I have nothing to add, if you like, we can merge 👍

@PSchmiedmayer PSchmiedmayer merged commit e699133 into StanfordSpezi:main Apr 17, 2024
15 of 16 checks passed
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.

Enable content adjustment in upcoming OnboardingStack steps
3 participants