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

Verticals popup menu on CourseUnitView #190

Merged
merged 12 commits into from
Dec 8, 2023

Conversation

forgotvas
Copy link
Contributor

@forgotvas forgotvas commented Dec 6, 2023

Added verticals popup menu to CourseUnitView. It closed by feature key VERTICALS_MENU_ENABLED of UI_COMPONENTS category, to test it need to use config from Config PR #157.

This issue related to: #156

Sample with key

Simulator.Screen.Recording.-.iPhone.15.-.2023-12-06.at.16.37.10.mp4

Sample with disabled or without key

Simulator.Screen.Recording.-.iPhone.15.-.2023-12-06.at.16.36.25.mp4

@miankhalid
Copy link

miankhalid commented Dec 7, 2023

@touchapp should we start the review of this PR or wait for:

  1. product input on the linked issue i.e. [iOS] Add drop-down menu with Sequence Units list #156?
  2. and the scope and acceptance criteria to be added on the same issue?

@forgotvas forgotvas linked an issue Dec 7, 2023 that may be closed by this pull request
private let landscapeTopSpacing: CGFloat = 75

let isDropdownActive: Bool = {
Container.shared.resolve(ConfigProtocol.self)?.uiComponents.isVerticalsMenuEnabled ?? false
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be better to pass this as an argument to the initializer or use ConfigProtocol from the viewModel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -180,4 +180,21 @@ public class CourseUnitViewModel: ObservableObject {
func trackFinishVerticalBackToOutlineClicked() {
analytics.finishVerticalBackToOutlineClicked(courseId: courseID, courseName: courseName)
}

func route(to vertical: CourseVertical) {
if let index = verticals.firstIndex(where: {$0.id == vertical.id}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Small improvement for readability
verticals.firstIndex(where: {$0.id == vertical.id}) -> verticals.firstIndex(where: { $0.id == vertical.id })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@volodymyr-chekyrta
Copy link
Contributor

Great feature!
Everything seems fine to me.
Just added two minor comments.

@forgotvas
Copy link
Contributor Author

fixed merge conflict and corrected code according @volodymyr-chekyrta comments

@volodymyr-chekyrta volodymyr-chekyrta merged commit c20f7ee into openedx:develop Dec 8, 2023
3 checks passed
@forgotvas forgotvas deleted the feat/unit-popup-view branch February 26, 2024 12:20
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.

[iOS] Add drop-down menu with Sequence Units list
3 participants