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

Unit progress navigation style #258

Merged
merged 41 commits into from
Feb 2, 2024

Conversation

rnr
Copy link
Contributor

@rnr rnr commented Jan 25, 2024

This PR is for next issues:

Added a progress bar at the top of the content screen. Each block can have one of four states:

  • selected completed
  • selected incomplete
  • completed unselected
  • incomplete unselected

If the progress line is present, the navigation style changes from vertical to horizontal.
This was done with COURSE_UNIT_PROGRESS_ENABLED FF - please use the configuration branch https://github.com/edx/edx-mobile-config/tree/saeed/openedX

Screen.Recording.2024-01-25.at.13.37.01.mov

eyatsenkoperpetio and others added 21 commits December 11, 2023 13:26
# Conflicts:
#	Core/Core/Configuration/Config/UIComponentsConfig.swift
#	Course/Course.xcodeproj/project.pbxproj
# Conflicts:
#	Core/Core.xcodeproj/project.pbxproj
#	Core/Core/Configuration/Config/UIComponentsConfig.swift
#	Core/Core/View/Base/WebUnitView.swift
#	Core/Core/View/Base/Webview/WebView.swift
#	Course/Course/Presentation/Unit/CourseUnitView.swift
#	Course/Course/Presentation/Unit/Subviews/WebView.swift
#	Theme/Theme/Theme.swift
chore: added one more state and change inset what fixes dark mode
set next and prev buttons in unit content to be horizontal if COURSE_UNIT_PROGRESS_ENABLED key is true
set next and prev animation in unit content to be horizontal if COURSE_UNIT_PROGRESS_ENABLED key is true
Copy link
Contributor

@saeedbashir saeedbashir left a comment

Choose a reason for hiding this comment

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

image

Shouldn't the arrow icon on the previous button in the unit navigation be on the left side of the text?

public var courseTopTabBarEnabled: Bool

init(dictionary: [String: Any]) {
courseNestedListEnabled = dictionary[Keys.courseNestedListEnabled.rawValue] as? Bool ?? false
courseBannerEnabled = dictionary[Keys.courseBannerEnabled.rawValue] as? Bool ?? false
courseUnitProgressEnabled = dictionary[Keys.courseUnitProgressEnabled.rawValue] as? Bool ?? false
Copy link
Contributor

@saeedbashir saeedbashir Jan 30, 2024

Choose a reason for hiding this comment

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

The Keys enum can implement protocol RawStringExtractable so that values can be extracted without mentioning the .rawValue from dictionary.

Comment on lines 12 to 14
public extension NSNotification {
static let blockCompletion = Notification.Name.init("block_completion")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Notifications are centralized in Core/Notification.swift. Can you please move this notification to there as well.

@@ -34,61 +35,134 @@ public struct WebView: UIViewRepresentable {
var webViewNavDelegate: WebViewNavigationDelegate?

var refreshCookies: () async -> Void

var isAddAjaxCallbackScript: Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

How about renaming it to addAjaxCallbackScript thoughts?

@@ -20,6 +20,7 @@ public struct WebView: UIViewRepresentable {

@Published var url: String
let baseURL: String
let ajaxProvider = AjaxProvider()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure whether we need to specify access levelfileprivate here or not because we are not using it for other variables, what do you think?

Comment on lines 15 to 18
public extension NSNotification {
static let blockChanged = Notification.Name.init("block_changed")
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this notification to Core/Notifications.

@@ -31,7 +31,9 @@ public class VideoPlayerViewModel: ObservableObject {
showError = errorMessage != nil
}
}


public var didUnitCompletion: (() -> Void)?
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused code can be removed.

@rnr
Copy link
Contributor Author

rnr commented Jan 30, 2024

image Shouldn't the arrow icon on the `previous` button in the unit navigation be on the left side of the text?

thank you for the highlighting
fixed

@saeedbashir
Copy link
Contributor

The progress gets lost in the same run if go back and come to the same screen.

Screen.Recording.2024-02-01.at.4.42.05.PM.mov

# Conflicts:
#	Podfile.lock
#	Theme/Theme/SwiftGen/ThemeAssets.swift
#	Theme/Theme/Theme.swift
# Conflicts:
#	Core/Core/Domain/Model/CourseBlockModel.swift
#	Course/Course.xcodeproj/project.pbxproj
#	Course/Course/Presentation/Container/CourseContainerViewModel.swift
#	Course/Course/Presentation/Outline/CourseOutlineView.swift
#	Course/Course/Presentation/Unit/CourseUnitView.swift
Comment on lines 378 to 383
if viewModel.blockChanged {
NotificationCenter.default.post(
name: .onRefreshCourse,
object: nil
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@eyatsenkoperpetio In which case this notification gets posted, I've checked this isn't calling when setting the blockChanged = true?

Copy link
Contributor

@saeedbashir saeedbashir left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@volodymyr-chekyrta volodymyr-chekyrta merged commit 02fcec9 into openedx:develop Feb 2, 2024
3 checks passed
@volodymyr-chekyrta
Copy link
Contributor

🚀 🚀 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants