-
Notifications
You must be signed in to change notification settings - Fork 48
feat: [FC-0047] Full-Bleed Header + Top Navigation #278
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
feat: [FC-0047] Full-Bleed Header + Top Navigation #278
Conversation
…, minor code refactoring
|
Thanks for the pull request, @PavloNetrebchuk! 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. |
335da1f to
42c81d5
Compare
42c81d5 to
9707ac8
Compare
…eader # Conflicts: # course/src/main/java/org/openedx/course/presentation/outline/CourseOutlineScreen.kt
|
Ready for product review |
marcotuts
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Product review only 👍
31ced6d to
07e2f11
Compare
9890076 to
655d2f4
Compare
discussion/src/main/java/org/openedx/discussion/presentation/topics/DiscussionTopicsFragment.kt
Show resolved
Hide resolved
course/src/main/java/org/openedx/course/presentation/outline/CourseOutlineScreen.kt
Show resolved
Hide resolved
HamzaIsrar12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swipe-to-refresh is still not working on the Dates and Discussion tabs.
course/src/main/java/org/openedx/course/presentation/dates/CourseDatesScreen.kt
Show resolved
Hide resolved
course/src/main/java/org/openedx/course/presentation/container/CourseHomeTab.kt
Outdated
Show resolved
Hide resolved
course/src/main/java/org/openedx/course/presentation/outline/CourseOutlineScreen.kt
Outdated
Show resolved
Hide resolved
b4a5157 to
64030e4
Compare
|
@HamzaIsrar12 You were correct about the pull-to-refresh. The data has been updated, but I forgot to hide the progress bar.😅 |
|
@k1rill @HamzaIsrar12 |
k1rill
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only question about viewModels left from my side, thank you
|
I'll finish my reviews by the end of the day today. 👍🏻 |
|
Hi @PavloNetrebchuk , I see you're addressing Kirill's feedback. Once you're done, could you please re-request the review from the |
6cd3747 to
a314e3d
Compare
…eader # Conflicts: # core/src/main/java/org/openedx/core/ui/theme/AppColors.kt # core/src/main/java/org/openedx/core/ui/theme/Theme.kt # core/src/main/res/values/strings.xml # core/src/openedx/org/openedx/core/ui/theme/Colors.kt
HamzaIsrar12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The happy scenarios are working fine ✨ ; it's just that in offline mode, the after-reloading states are a bit buggy. However, since we have a ticket for properly handling the error state, I think we can ignore them for now.
Other than that, I've only requested minor refactoring and formatting in the code, which I believe we won't be able to address later on.
core/src/main/java/org/openedx/core/presentation/course/CourseContainerTab.kt
Outdated
Show resolved
Hide resolved
course/src/main/java/org/openedx/course/presentation/container/CourseContainerFragment.kt
Outdated
Show resolved
Hide resolved
course/src/main/java/org/openedx/course/presentation/container/CourseContainerFragment.kt
Outdated
Show resolved
Hide resolved
course/src/main/java/org/openedx/course/presentation/container/CourseContainerFragment.kt
Outdated
Show resolved
Hide resolved
course/src/main/java/org/openedx/course/presentation/container/CourseContainerFragment.kt
Show resolved
Hide resolved
...ussion/src/main/java/org/openedx/discussion/presentation/topics/DiscussionTopicsViewModel.kt
Outdated
Show resolved
Hide resolved
HamzaIsrar12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 🏎️ ✨
|
We discussed in the strat meeting that this should be good to merge, @volodymyr-chekyrta. |
@e0d Thank you! I don't have permission to merge it without @k1rill's approval. |
Discussed in the team meeting, capacity is a blocker, we have multiple thumbs.
|
@PavloNetrebchuk 🎉 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.
Android < 12
https://github.com/openedx/openedx-app-android/assets/127732735/5cc29cf1-751e-443b-a54f-f129063f6b88
🚨 Breaking changes 🚨:
The
COURSE_BANNER_ENABLEDandCOURSE_TOP_TAB_BAR_ENABLEDfeature flags have been removed.