[Horizon] Add navigation bar #3034
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ignore-commit-lint]
Ahmed-Naguib93
requested review from
szabinst,
ktheory,
JacksonHowe and
reabbotted
as code owners
Horizon Build QR Code: |
❌ XCTest failed: CoreTests/AssignmentCellViewModelTests/testSubmissionStatusAndIconAndColor❌ XCTest failed: CoreTests/DiscussionDetailsViewControllerTests/testAutomaticRead❌ XCTest failed: CoreTests/DiscussionDetailsViewControllerTests/testLayout❌ XCTest failed: CoreTests/DiscussionDetailsViewControllerTests/testShowEntry❌ XCTest failed: CoreTests/DiscussionDetailsViewControllerTests/testShowReplies❌ XCTest failed: CoreTests/DiscussionDetailsViewControllerTests/testStudentGroupTopic❌ XCTest failed: CoreTests/DiscussionDetailsViewControllerTests/testStudentGroupTopicWhenUserNotInAGroup❌ XCTest failed: CoreTests/DiscussionDetailsViewControllerTests/testTeacherGroupTopic |
reabbotted
reviewed
Dec 18, 2024
| .scrollIndicators(.hidden, axes: .vertical) | ||
| .background(Color.backgroundLight) | ||
| .onFirstAppear { viewModel.viewController = viewController} |
There was a problem hiding this comment.
I don't love the pattern of requiring the view to set a property on the view model. I think it's better just to pass the viewController as an argument to the notebookDidTap method. That way it's always clear what is required. Otherwise it can be unclear to someone using the view model that they need to set that property before it'll work.
There was a problem hiding this comment.
Ok but i need to pass the closure direct to view model, because the navigation logic must be set in viewModel.
There was a problem hiding this comment.
It would just be one of the parameters passed in the onEvent handler:
func onEvent(event: HorizonUI.NavigationBar.Trailing.Event, viewController: WeakViewController)
You might also consider creating a separate ViewModel to pair with the HorizonUI.NavigationBar.Trailing and putting the onEvent handler in there. Otherwise we must handle the navigation logic in the ViewModel for every View that uses the HorizonUI.NavigationBar.Trailing view.
There was a problem hiding this comment.
If I'm being honest though, the amount of logic that would go into the Trailing view controller is so small and unlikely to be broken, I'd probably just put it into the view. So it's up to you what you want to do there.
There was a problem hiding this comment.
No, HorizonUI is just for creating UI only and shouldn't have any logic in it.
There was a problem hiding this comment.
Since this component will be used only on the dashboard, I'd simply pass 3 named () -> Void closure to the HorizonUI.NavigationBar.Trailing initializer then we can have:
viewModel.notebookDidTap(viewController)
viewModel.notificationDidTap(viewController)
...the rest
that is readable easily.
reabbotted
reviewed
Dec 18, 2024
Horizon/Horizon/Sources/Features/Dashboard/View/DashboardViewModel.swift
Outdated
Show resolved
Hide resolved
reabbotted
reviewed
Dec 18, 2024
...ges/HorizonUI/Sources/HorizonUI/Sources/Components/NavigationBar/TrailingNavigationBar.swift
Show resolved
Hide resolved
reabbotted
previously approved these changes
Dec 18, 2024
szabinst
requested changes
Jan 6, 2025
...ages/HorizonUI/Sources/HorizonUI/Sources/Components/NavigationBar/LeadingNavigationBar.swift
Outdated
Show resolved
Hide resolved
| .scrollIndicators(.hidden, axes: .vertical) | ||
| .background(Color.backgroundLight) | ||
| .onFirstAppear { viewModel.viewController = viewController} |
There was a problem hiding this comment.
Since this component will be used only on the dashboard, I'd simply pass 3 named () -> Void closure to the HorizonUI.NavigationBar.Trailing initializer then we can have:
viewModel.notebookDidTap(viewController)
viewModel.notificationDidTap(viewController)
...the rest
that is readable easily.
szabinst
approved these changes
Jan 7, 2025
Ahmed-Naguib93
deleted the
feature/horizon-CLX-282-top-nav-bar-components
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Navigation bar
https://instructure.atlassian.net/browse/CLX-282