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

Course tab bar from the bottom of the screen move to the top #181

Merged
merged 15 commits into from
Jan 9, 2024

Conversation

eyatsenkoperpetio
Copy link
Contributor

@eyatsenkoperpetio eyatsenkoperpetio commented Nov 29, 2023

In course screen removed tab bar and create new sliding top tab bar

add to configuration:

UI_COMPONENTS:
COURSE_TOP_TAB_BAR_ENABLED: true

Screenshot 2023-11-30 at 15 03 06 Screenshot 2023-11-30 at 15 02 42

# Conflicts:
#	Course/Course/Presentation/Container/CourseContainerView.swift
# Conflicts:
#	Core/Core.xcodeproj/project.pbxproj
# Conflicts:
#	Core/Core.xcodeproj/project.pbxproj
#	Course/Course/Presentation/Container/CourseContainerView.swift
@miankhalid
Copy link

miankhalid commented Nov 30, 2023

@eyatsenkoperpetio can you add a brief description of what this PR is accomplishing? (along with some screenshots/videos of the updated behavior)

also, it'll be great if you can link the PR with an issue on the Roadmap that its addressing, thanks!

@eyatsenkoperpetio eyatsenkoperpetio linked an issue Nov 30, 2023 that may be closed by this pull request
@eyatsenkoperpetio
Copy link
Contributor Author

Screen.Recording.2023-11-30.at.09.37.38.mov

@miankhalid
Copy link

@eyatsenkoperpetio it'll be great if you can add all the relevant info to the PR's description instead of separate comments.

@eyatsenkoperpetio eyatsenkoperpetio changed the title slide top bar Course tab bar from the bottom of the screen move to the top of the screen Nov 30, 2023
@eyatsenkoperpetio eyatsenkoperpetio changed the title Course tab bar from the bottom of the screen move to the top of the screen Course tab bar from the bottom of the screen move to the top Nov 30, 2023
@saeedbashir
Copy link
Contributor

In the navbar overhaul, when we moved the course menu bar (bottom bar) to the top, we actually fixed the course header (containing course name, organization, end date, etc.) for all the tabs and positioned the menu bar (top bar) below the course header view.

In this PR, the course menu bar simply moved to the top without changing the behavior or course header view. @moeez96 I'm not sure what path we will be following here to achieve the current behavior (edX app behavior) for the course dashboard.
cc @miankhalid @touchapp

@moiz994
Copy link

moiz994 commented Dec 4, 2023

@saeedbashir imo we should follow the current edX design pattern. Schema is working on the design for this following the prod edX app design pattern but it isn't yet complete. Would it cause any kind of issues if we merge the menu bar before the change in the course header?

@saeedbashir
Copy link
Contributor

Would it cause any kind of issues if we merged the menu bar before the change in the course header?

@moiz994 It won't cause any issues, but I'm not sure what the design team is thinking or how we will be going with the new design. I'm waiting for a design response on the attached issue to clarify the requirements.

@touchapp
Copy link

touchapp commented Dec 7, 2023

Hey @saeedbashir can you show the updates you have made to the header? Perhaps make a similar video as @eyatsenkoperpetio ?

@eyatsenkoperpetio eyatsenkoperpetio requested a review from rnr December 7, 2023 15:38
@saeedbashir
Copy link
Contributor

saeedbashir commented Dec 8, 2023

Hey @saeedbashir can you show the updates you have made to the header? Perhaps make a similar video as @eyatsenkoperpetio ?

@touchapp The header is completely redesigned; for this PR, what we can do is we can use the existing header (course image and info) but make it consistent across all the tabs and move the tabs below the course image. Thoughts?
I'm attaching the video from the edX production app.

Note: Given that the cross button (top right) would be replaced with the back arrow on top left, because we aren't following presentation mode in the new app.

RPReplay_Final1702016334.MP4

# Conflicts:
#	Core/Core.xcodeproj/project.pbxproj
#	Core/Core/Configuration/Config/FeaturesConfig.swift
#	Course/Course/Presentation/Container/CourseContainerView.swift
rnr
rnr previously approved these changes Dec 11, 2023
@miankhalid
Copy link

Not sure what's happening here. Multiple folks have approved the PR but:

  1. there's an active discussion going on in the linked issue [iOS] Move Course menu from bottom to top #154
  2. plus there's this item that needs a response as well: Course tab bar from the bottom of the screen move to the top #181 (comment)

rnr
rnr previously approved these changes Dec 13, 2023
# Conflicts:
#	Core/Core.xcodeproj/project.pbxproj
#	Core/Core/Configuration/Config/UIComponentsConfig.swift
rnr
rnr previously approved these changes Dec 14, 2023
@volodymyr-chekyrta
Copy link
Contributor

Tests failed because of timeout.
Re-runed pipeline, let's see.

@volodymyr-chekyrta
Copy link
Contributor

Not sure what's happening here. Multiple folks have approved the PR but:

  1. there's an active discussion going on in the linked issue [iOS] Move Course menu from bottom to top #154
  2. plus there's this item that needs a response as well: Course tab bar from the bottom of the screen move to the top #181 (comment)

@touchapp @rnr
Code looks good to me, but can someone please react to Khalid's message?

# Conflicts:
#	Core/Core.xcodeproj/project.pbxproj
# Conflicts:
#	Core/Core.xcodeproj/project.pbxproj
@miankhalid
Copy link

miankhalid commented Jan 8, 2024

@moiz994 @marcotuts what's the decision on this? should we go ahead with these changes? or dismiss this PR?

@moiz994
Copy link

moiz994 commented Jan 8, 2024

@miankhalid as per the last design sync, the design for the course dashboard is still in progress and we could not make a final decision on the way forward for it. If this is a feature flag-based implementation, I'd say go ahead with merging it for now.

@volodymyr-chekyrta volodymyr-chekyrta merged commit b0b829b into openedx:develop Jan 9, 2024
3 checks passed
@forgotvas forgotvas deleted the feat/slide_topbar branch June 20, 2024 14:43
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] Move Course menu from bottom to top
7 participants