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

Handle Education Video fetching failure with content unavailable view #90

Closed
wants to merge 7 commits into from

Conversation

PaulGoldschmidt
Copy link

@PaulGoldschmidt PaulGoldschmidt commented Sep 9, 2024

Handle Education Video fetching failure with content unavailable view with ContentUnavailableView

♻️ Current situation & Problem

This PR implements the ContentUnavailableView for the education video fetching failure(s) and should fix the issue #84.

⚙️ Release Notes

The Education tab now displays a "Content Unavailable" view when no educational videos are present, improving user feedback. Empty video collections are filtered out, ensuring users only see sections with actual content 🎥.
grafik

✅ Testing

Tests pass just fine (iOS 17.4)

🚸 Info

This is my first PR in this project, I'm happy for any improvments and feedback for my code {style} 🙆‍♂️

Code of Conduct & Contributing Guidelines

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

@PaulGoldschmidt PaulGoldschmidt changed the title Handle Education Video fetching failure with content unavailable view with ContentUnavailableView Handle Education Video fetching failure with content unavailable view Sep 9, 2024
Copy link
Contributor

@pauljohanneskraft pauljohanneskraft left a comment

Choose a reason for hiding this comment

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

Hey - thank you very much for your contribution! I gave one small change request and then it should be good to go. Let me know what you think!

ENGAGEHF/Education/VideoList.swift Outdated Show resolved Hide resolved
@PaulGoldschmidt
Copy link
Author

Hi @pauljohanneskraft,

Thank you for that helpful suggestion, I really appreciate your feedback! I've added another condition at the ContentUnavailableView to Education.swift while reverting VideoList.swift to address the double implementation. Could you please review the PR again and let me know if any further changes are needed? Also, I'm open to exploring alternative implementations of the ContentUnavailableView for this case if you think that would be better 🕵️‍♂️.

Thanks alot!
-Paul

Copy link
Author

@PaulGoldschmidt PaulGoldschmidt left a comment

Choose a reason for hiding this comment

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

Should be mitigated by implementing ContentUnavailableView only in Education.swift.

Copy link
Contributor

@pauljohanneskraft pauljohanneskraft left a comment

Choose a reason for hiding this comment

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

Looks good to me - one minor improvement idea, if you agree with the change 😊 Thank you for the contribution!

ENGAGEHF/Education/Education.swift Outdated Show resolved Hide resolved
@PaulGoldschmidt
Copy link
Author

Seperating the filtering logic for sure benefits the readability and (I would assume) have a less performance overhead.
More than happy to see it implemented the way you proposed, thanks for the suggestion @pauljohanneskraft! I'll be happy to take the learnings from this PR into future work in the ecosystem.

@PaulGoldschmidt
Copy link
Author

Yikes, I've just realized that commit signing wasn't configured fully in my Xcode instance on the new hardware. I propose closing this pull request and submitting a new one with properly signed commits if that works for you. Sorry! :shipit:

@pauljohanneskraft
Copy link
Contributor

Yikes, I've just realized that commit signing wasn't configured fully in my Xcode instance on the new hardware. I propose closing this pull request and submitting a new one with properly signed commits if that works for you. Sorry! :shipit:

No worries - feel free to either force-push this branch or open a new PR, whichever is easier!

…nce with issue StanfordBDHG#84:

Update Education view to handle empty video collections and modification of VideoList to display message when no videos are available
… check to catch that the videoCollections is empty.
@PaulGoldschmidt
Copy link
Author

Done, forced pushed and reenabled commit signing :)

@pauljohanneskraft
Copy link
Contributor

pauljohanneskraft commented Sep 13, 2024

Perfect - thank you! 😊 I'm not sure whether the CI pipeline will be successful, but once #92 is merged, we should be able to merge the main branch into this one and it should work out fine! I will try to make that happen by EOD and let you know!

@PaulGoldschmidt
Copy link
Author

Seems like the UI tests are failing because the buttons for different tabs are not found - from what I can tell, these issues don't seem directly related to the changes in this PR. I'll keep an eye on the test results over the next few days and dig deeper if the problems persist. If anyone has insights or wants me to take a closer look at the test outputs and the tests directly, just let me know.

@pauljohanneskraft
Copy link
Contributor

The tests are failing because the app is not able to automatically sign in for some reason. As I couldn't find the root cause of the problem, I'm trying to just recreate the changes made here in a separate PR.

Since the pipelines of #94 seem to be working fine, I'm thinking maybe this is related to the changes coming from a fork? I will investigate further, just wanted to give a quick update.

@pauljohanneskraft
Copy link
Contributor

pauljohanneskraft commented Sep 25, 2024

I think it might be related to secrets access of the CI when running with the changes from your fork. For now, I have duplicated the changes in #94, I hope it is fine for you if we merge it like that, sorry 😅

If you want to make further changes to this repository, please let us know and we may be able to grant you write access to the repository, so that you can work directly on it rather than working on a fork.

CC: @PSchmiedmayer

@PaulGoldschmidt
Copy link
Author

Hi @pauljohanneskraft, thanks for digging into this issue - I was suspecting something like missing secrets (we're not the first to experience this kind of issue), I assume reopening this PR just like you've done is the best approch here.

While this minor improvement was a great introduction to SPEZI, I'll be focusing my contributions mainly on the framework itself going forward. I'll let you know if I need write access in the future.
I'm closing this PR now. Thanks again for your help.

pauljohanneskraft added a commit that referenced this pull request Oct 3, 2024
# Add ContentUnavailableView to Education page

## ♻️ Current situation & Problem
This is just a copy of the changes made in #90 - This is mostly trying
to debug why tests are failing by simply recreating the same changes and
checking if the CI reacts differently here...


## ⚙️ Release Notes 
- Handle Education Video fetching failure with content unavailable view
(#90) by @PaulGoldschmidt


## 📚 Documentation
*Please ensure that you properly document any additions in conformance
to [Spezi Documentation
Guide](https://github.com/StanfordSpezi/.github/blob/main/DOCUMENTATIONGUIDE.md).*
*You can use this section to describe your solution, but we encourage
contributors to document your reasoning and changes using in-line
documentation.*


## ✅ Testing
*Please ensure that the PR meets the testing requirements set by CodeCov
and that new functionality is appropriately tested.*
*This section describes important information about the tests and why
some elements might not be testable.*


### Code of Conduct & Contributing Guidelines 

By submitting creating this pull request, you agree to follow our [Code
of
Conduct](https://github.com/StanfordBDHG/.github/blob/main/CODE_OF_CONDUCT.md)
and [Contributing
Guidelines](https://github.com/StanfordBDHG/.github/blob/main/CONTRIBUTING.md):
- [x] I agree to follow the [Code of
Conduct](https://github.com/StanfordBDHG/.github/blob/main/CODE_OF_CONDUCT.md)
and [Contributing
Guidelines](https://github.com/StanfordBDHG/.github/blob/main/CONTRIBUTING.md).
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
Status: Done
Development

Successfully merging this pull request may close these issues.

Handle Education Video fetching failure with content unavailable view.
3 participants