feat: [FC-0047] Full-Bleed Header + Top Navigation #385
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @IvanStepanok! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
volodymyr-chekyrta
changed the title
volodymyr-chekyrta
added
the
product review
|
Ready for product review |
|
Issues with loading indicator:
|
|
Issues with Scroll
Screen.Recording.2024-04-18.at.08.36.15.mov
Screen.Recording.2024-04-18.at.08.35.22.mov |
|
The progress bar location is odd if we use pull to refresh while data is loading. and If we interact with UI white loading the bleeding header isn't working properly. loading.and.scroll.issues.mov |
@rnr Thanks for your review!🙌
Scroll issues
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-04-18.at.18.22.28.mp4
Screen.Recording.2024-04-18.at.18.25.03.movIf the content height in ScrollView is not enough to collapse the header, collapsing mode are disabled. Thanks for paying attention to this bug, it's very important.🙌 @saeedbashir Thanks for your review, too!❤️
Screen.Recording.2024-04-18.at.18.35.21.movIf you discover any new bugs, I'd be delighted to fix them! |
I think we need to disable collapse mode for the 'More' tab as it tries to collapse, but we have two rows (Distributions and Announcements) at the moment. Thank you Screen.Recording.2024-04-19.at.11.28.27.mov |
|
Is it a design decision to disable this collapse mode for iPad in general? Screen.Recording.2024-04-19.at.11.37.37.mov |
| @@ -0,0 +1,173 @@ | |||
| // | |||
| // TopHeaderView.swift | |||
There was a problem hiding this comment.
How about giving it a more meaningful name like course header view or some as per its functionality Thoughts?
There was a problem hiding this comment.
Renamed to CourseHeaderView
| @ObservedObject var viewModel: CourseContainerViewModel | ||
| var title: String | ||
| @Binding var collapsed: Bool | ||
| var containerWidth: CGFloat | ||
| var animationNamespace: Namespace.ID | ||
| @Binding private var isAnimatingForTap: Bool | ||
| @Environment(\.isHorizontal) private var isHorizontal | ||
|
|
||
| let collapsedHorizontalHeight: CGFloat = 230 | ||
| let collapsedVerticalHeight: CGFloat = 260 | ||
| let expandedHeight: CGFloat = 300 |
There was a problem hiding this comment.
All these variables can be private.
| private struct GeometryName { | ||
| static let backButton = "backButton" | ||
| static let topTabBar = "topTabBar" | ||
| static let blurSecondaryBg = "blurSecondaryBg" | ||
| static let blurPrimaryBg = "blurPrimaryBg" | ||
| static let blurBg = "blurBg" | ||
| } |
There was a problem hiding this comment.
Do we need a struct here, how about converting it to an enum?
There was a problem hiding this comment.
Thanks! Good idea
| } | ||
| } | ||
| .disabled(true) | ||
| .ignoresSafeArea() |
There was a problem hiding this comment.
The code needs formatting here.
| .ignoresSafeArea(edges: .top) | ||
| } | ||
|
|
||
| private func topTabBar(containerWidth: CGFloat) -> some View { |
There was a problem hiding this comment.
How about renaming it to courseMenuBar or courseTabbar? thoughts?
|
|
||
| import SwiftUI | ||
|
|
||
| public struct RefreshProgressView: View { |
There was a problem hiding this comment.
I guess it's being used for pull to refresh, how about renaming it as per functionality thoughts?
|
|
||
| import SwiftUI | ||
|
|
||
| public struct ResponsiveView: View { |
There was a problem hiding this comment.
Is it possible to give it a meaningful name as every view is by default responsive view.
There was a problem hiding this comment.
Renamed to DynamicOffsetView. Good?
| private let expandedHeight: CGFloat = 240 | ||
| private let coordinateBoundaryLower: CGFloat = -115 | ||
|
|
||
| @State var lastHeight: CGFloat = 240 |
| .overlay( | ||
| GeometryReader { geometry -> Color in | ||
| guard idiom != .pad, enableCollapsing else { | ||
| return Color.clear |
There was a problem hiding this comment.
Color is redundant, you can directly use .clear.
Screen.Recording.2024-04-21.at.21.54.55.mov@rnr, @saeedbashir, thanks for the review, I appreciate it. Feel free to reach out if you find any new bugs or have questions. Thanks again! |
Yes, it is approved with @sdaitzman |
|
Header bar hides logo by half or more on iPad:
UPDATE: It's expected behavior as teams decided on demo call. |
|
Back button in wrong position for pages with header on iPad:
|
|
Controller with header doesn't have navigation title: 2024-04-22.12.44.56.mov |
|
Resume button doesn't have paddings on top:
|
|
Pull to refresh indicator disappeared from videos tab: |
| @@ -40,7 +40,6 @@ struct ContinueWithView: View { | |||
| UnitButtonView(type: .continueLesson, action: action) | |||
| .frame(width: 200) | |||
| } .padding(.horizontal, 24) | |||
| .padding(.top, 32) | |||
There was a problem hiding this comment.
Are you sure that we don't need that?
|
Thanks @forgotvas, all your comments have been addressed! I've also vertically centered the banner so that it crops more evenly. Screen.Recording.2024-04-22.at.21.51.07.mov |
|
Thanks, @IvanStepanok, just one was skipped - back button has wrong position it should be at same position as on content page:
|
|
@forgotvas Thanks for the good idea! The position of the back button is consistent with the design, but I asked Sam Deitzman if it was possible to change the position and they confirmed that it would indeed be better. The Back button is now on the left. Screen.Recording.2024-04-23.at.10.34.20.mov |
|
@IvanStepanok 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
This PR introduces updated navigation and banner for the course home.
Light and Dark theme:
In the horizontal state, the top bar is always collapsed.
The iPad version doesn't have a collapsing effect.
🚨 Breaking changes 🚨:
The
COURSE_BANNER_ENABLEDandCOURSE_TOP_TAB_BAR_ENABLEDfeature flags have been removed.