StanfordSpezi / SpeziOnboarding

Spezi Onboarding module to inform a user or retrieve consent for a study participation
https://swiftpackageindex.com/StanfordSpezi/SpeziOnboarding/documentation/
MIT License
11 stars 5 forks source link

Content adjustment for upcoming onboarding steps #46

Closed felixschlegel closed 2 months ago

felixschlegel commented 3 months ago

Content adjustment for upcoming onboarding steps

:recycle: 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.

:gear: Release Notes

:books: Documentation

:white_check_mark: Testing

:pencil: Code of Conduct & Contributing Guidelines

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

philippzagar commented 3 months ago

@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.

felixschlegel commented 2 months ago

@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.

felixschlegel commented 2 months ago

@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 👍