feat: Course Level Error Handling for Empty States #477
Conversation
| { | ||
| "images" : [ | ||
| { | ||
| "filename" : "campaign_24dp_FILL0_wght400_GRAD0_opsz24.svg", |
There was a problem hiding this comment.
Could you please change the file name as per the asset name?
| case noVideos(error: String, image: SwiftUI.Image) | ||
| case noHandouts(error: String, image: SwiftUI.Image) | ||
| case noAnnouncements(error: String, image: SwiftUI.Image) | ||
| case noCourseDates(error: String, image: SwiftUI.Image) | ||
| case noCourseware(error: String, image: SwiftUI.Image) |
There was a problem hiding this comment.
We are passing the same data in all the cases, how about defining a single case for all the cases and use that? Thoughts?
There was a problem hiding this comment.
I was thinking the same thing, but I opted for this approach to accommodate any future type-specific UI requirements. We can opt for single case for now. Thoughts?
There was a problem hiding this comment.
Opting for a single case is good at the moment. We will update it in the future as per the need.
| GeometryReader { proxy in | ||
| VStack(spacing: 20) { | ||
| Spacer() | ||
| switch errorType { |
There was a problem hiding this comment.
It seems bit odd to use the switch again under a case of the same switch. How about using a single GeometryReader and configure it according to the need.
There was a problem hiding this comment.
Thank you for pointing that out. I have fixed it.
| GeometryReader { proxy in | ||
| VStack { | ||
| ScrollView { | ||
| DynamicOffsetView( | ||
| coordinate: $coordinate, | ||
| collapsed: $collapsed, | ||
| viewHeight: $viewHeight | ||
| ) | ||
|
|
||
| FullScreenErrorView( | ||
| type: .noCourseDates( | ||
| error: CourseLocalization.Error.courseDateUnavailable, | ||
| image: CoreAssets.information.swiftUIImage | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Is it possible to configure FullScreenErrorView in a way that we can directly use it like we are using OfflineSnackBarView?
| CoreAssets.noWifi.swiftUIImage | ||
| switch errorType { | ||
| case .noVideos(error: let error, image: let image), | ||
| .noHandouts(error: let error, image: let image), |
There was a problem hiding this comment.
Please remove the extra indentation
There was a problem hiding this comment.
XCode indenting it like this way.
| .padding(.top, 8) | ||
| .frame( | ||
| maxHeight: .infinity, | ||
| alignment: .topLeading) |
There was a problem hiding this comment.
Could you please move bracket on new line?
| viewModel.router.showHandoutsUpdatesView( | ||
| handouts: viewModel.handouts, | ||
| announcements: nil, | ||
| router: viewModel.router, | ||
| cssInjector: viewModel.cssInjector) | ||
| cssInjector: viewModel.cssInjector, | ||
| type: type) |
There was a problem hiding this comment.
Could you please move bracket on new line?
| announcements: viewModel.updates, | ||
| router: viewModel.router, | ||
| cssInjector: viewModel.cssInjector, | ||
| type: type) |
There was a problem hiding this comment.
Could you please move bracket on new line?
|
@shafqat-muneer Screen.Recording.2024-07-02.at.16.01.12.mov |
@rnr Thank you for pointing that out. I have fixed it. |
|
@saeedbashir @rnr Feedback addressed. Ready for another round of review. 🎉 |
| error: DiscussionLocalization.Error.unableToLoadCourseContent, | ||
| image: CoreAssets.information.swiftUIImage |
There was a problem hiding this comment.
Instead of passing message and icon from everywhere, how about just setting the type and define a method in FullScreenErrorView for message and icon. You can keep a generic type that will take message and icon as in input for future.
There was a problem hiding this comment.
The FullScreenErrorView is utilized across various modules, with its implementation located in the Core module to enable application-wide usage. Embedding messages within the full screen would necessitate moving module-specific errors into the core module, which is not ideal. Therefore, I suggest passing the message and icon instead.
shafqat-muneer
force-pushed
the
Shafqat/LEARNER-10020
branch
from
152656a
to
d061772
Compare
|
@saeedbashir Feedback addressed 🎉 |
|
@volodymyr-chekyrta Awaiting your final review prior to merging. |
|
We're closing this PR and moving these changes to our own fork, as they are critical for us. After our release, we hope to find a way to contribute these changes back, thanks! |
LEARNER-10020: iOS - Course Level Error Handling for Empty States
Portrait Mode:
Landscape Mode: